Add a standard wallet interface

[piyalbasu]

Background
Many browser wallets expose an API so dapps can interact with a user's wallet. This allows a dapp to ask a wallet to do things like transmit a public key or sign an XDR.

Problem
Right now wallets all implement this API differently, which can make building a dapp or an SDK difficult; a dev must account for all these differences and manually integrate different wallet API's in order to support as many wallets as possible. This will conceivably become more complex as more wallets enter the ecosystem.

Proposal
The ecosystem agrees to a standard wallet API interface to make wallet integration easier. If all wallets agree to a minimum standard interface, a dapp or SDK developer could reliably infer what methods will be supported and what the function signature will be.

Example API interface:

{
  getPublicKey: () => Promise<publicKey as string>;
  signTransaction: (xdr: string) => Promise<signedXdr as string>;
  signAuthEntry: (authEntryXdr: string) => Promise<signedAuthEntry as string>
}

Relevant discord conversation: https://discord.com/channels/897514728459468821/1019346446014759013/1206810627377467413

[Shaptic]

It seems like the Discord discussion is pretty dated, so I'll offer some thoughts on this here, also to get this discussion going again.

I like the proposed interface, but I'd like to suggest an additional generic signing method that could actually encapsulate the other two and also be used for arbitrary signing purposes:

interface Wallet {
  sign: (arbitraryB64: string) => Promise<string>;
}

(Internally, an implementation could have something like signTransaction = (xdr: string) => { /* verify xdr is a Transaction, then just */ return this.sign(xdr); }.)

---

Also, should we discuss the trade-off between allowing encoded XDR passed in vs. the actual XDR object? I think encoded is fine, but we need to be sure to communicate to the implementers that they should NOT trust the input string to be well-formed or even necessarily represent an xdr.Transaction, for example. In other words, they should be very robust to failure and skeptical of the input. This wouldn't apply to the sign method, of course, besides maybe validating that it's base64?

[piyalbasu]

I like the idea of a generic signer.

We have run into some issues passing the actual XDR object using window.postMessage. It appears postMessage strips the custom prototypes and will just pass an object with plain key/values. We've found it's more reliable to pass the xdr string and decode/encode at the destination

[Shaptic]

Ah, that's a great reason! Keeping it encoded is fine, then, we'll just have to remember the "tell people to not trust the input string" part.

[leighmcculloch]

Another motivation to stick with XDR is that to rely on the xdr.Transaction object is to make the interface only work with applications that use the js-stellar-sdk/base lib, which an application may not be using.

[leighmcculloch]

A generic signing function presents a couple challenges:

1. It'd need a parameter to discriminate which XDR type was being passed in. The discriminant is needed because XDR encoded does not self-identify. An XDR binary stores no data to tell you what type it is. In the case of TransactionEnvelope and AuthEntry you probably can tell by trying to decode one, failing, and trying to decode the other one. But if that works reliably, it is luck, as there's nothing in XDR that guarantees that distinction is possible.

   interface Wallet {
      sign: (type: string, arbitraryB64: string) => Promise<string>;
   }

   When there are two separate functions the functions act as a discriminant, so really the generic signing function with the extra parameter is just moving the discriminant, not eliminating it.

   I think the generic function is more ergonomic, but I think it needs the discriminant still.

2. It makes it easier for a wallet developer to do an unsafe thing, because a wallet developer could look at that interface and think that they should support signing any arbitrary data. To do so would probably allow an application to get the user to sign things they shouldn't. So +100 to @Shaptic's comments about not trusting the input, it would be important for the wallet to be strict at only signing data that does decode to a tx or auth entry.

[piyalbasu]

@leighmcculloch That's great context, thanks. Without investigating, I had assumed that maybe there was a way to identify a xdr type from the xdr itself. Also very interesting that trying to decode as different types until success wouldn't reliably work either

We can still create a generic sign with the discriminant param. It is a little more dev experience friendly to just be able to change a param in the code rather than having to call a different method

[earrietadev]

If we are adding extra methods, I would like to include an extra one:

• signMessage in the same way Albedo does it (or at least something similar).

I also prefer separated functions instead of a single generic function because it's easier for a wallet implementing something unique that they only have (like albedo with its messages signing features) and dapp developers can easily decide if they want to support that or not by just adding that extra function.

But that being said the more "features" we add (sighAuthEntry included) the more we will reduce the support for non stellar native wallets like those using wallet connect (Lobstr), hardware wallets, airgapped wallets, etc so at the end of the day devs will still need to handle different interfaces no matter what

[orbitlens]

I have mixed feelings about such an interface. It's simple and unified, and that's great. On the other hand, I really don't like the idea of returning plain strings from the method calls.

Can we change the interface to return objects (with the predefined set of fields) instead of strings? For example,

{
  getPublicKey: () => Promise<{publicKey: string}>,
  signTransaction: (xdr: string) => Promise<{signedXdr: string}>,
  signAuthEntry: (authEntryXdr: string) => Promise<{signedAuthEntry: string}>,
  signMessage: (message: string) => Promise<{signedMessage: string}>
}

Motivation:

• More flexible -- wallet developers can add additional fields to the response object without breaking the interface.
• It's future-proof and extendable -- if needed, the returned object format can can be extended in the future by adding additional fields, or removing obsolete fields.
• Backwards-compatible -- we'll be able to extend Albedo client lib bindings to support this standard without breaking other developers' code that relies on a specific format that we currently return.

[orbitlens]

Aside from the successful result standardization, we also have to define the standardized error output format.
External apps that integrate the wallet interface need to react on possible errors. Returning a generic error is bad idea, because every wallet will have a different error descriptions attached to it. The average app reaction on an error is to show the notification with error text. But depending on the connected wallet, the error text may be quite different. Ranging from "Failed" to "fetch() request error at line 13:53". This is non-informative for end-users and doesn't allow the client app to employ different logic for handling different types of errors (for example, in case of the Horizon error the client app might decide to resubmit the request again).

Albedo client lib defines several standard errors for different scenarios. I'm not advocating for sticking with our vendor format, but it would be nice to have a few basic standard errors defined in a minimalistic format, e.g.

{
  message: string,    // general description message returned to the client app
  code: number,       // unique error code
  ext: Array<string>  // optional extended details
}

Proposed standard errors:

{
  unhandled: { // internal wallet error, likely caused by the wallet logic itself
    message: 'Unhandled error occurred.',
    code: -1
  },
  external: { // external service (Horizon, RPC, etc.) returned an error 
    message: 'External error occurred.',
    code: -2,
    ext: ['Operation 2: Invalid "amount" value.', "Operation 5: Asset issuer is required."] // malformed tx error example
  },
  invalid: { // client app request is invalid (wrong parameters, invalid transaction XDR, etc.)
    message: 'Request is invalid.',
    code: -3,
    ext: ['Invalid transaction XDR'] // example error
  },
  rejected: { // user rejected the request, client app should not try to retry the request
    message: 'Rejected by the user.',
    code: -4
  }
}

[orbitlens]

One more thing to consider. Multi-account wallets in some cases need an option to identify the specific account to utilize for the request.
For example, I have 10 different accounts in my wallet. When the client app requests the public key, I can save a "session" for this app, and imply that e.g. signTransaction() request relates to that exact account.

However, if the user interacted with the app from two different accounts (for example, traded with DEX from two different accounts), I have a problem. The obvious solution is to show the list (or dropdown) of all wallet accounts with pre-selected the most recent account. Yet allowing an app to request signature for a specific account makes the UX much more streamlined on the wallet side, decreasing the possibility of a human error and saving a few clicks.

How about adding a second optional parameter publicKey into signTransaction(), signAuthEntry(), signMessage() methods to simplify interaction for multi-account wallets?

[earrietadev]

I agree with this and maybe instead of adding a second parameter we send an object as first parameter right away, it gives more flexibility if in the future we want to add something else

[orbitlens]

Lastly, kudos to @earrietadev for maintaining the StellarWalletsKit lib that allows client apps to avoid complexity of dealing with different wallets by abstracting the interface.

[leighmcculloch]

One thing I notice about the proposed API is that it doesn't expose any concept of a network, or a network passphrase. If a web app knew that it was only deployed to testnet or pubnet, how would it communicate the supported network, or see what network the wallet supported?

[earrietadev]

All 4 extension wallets (Albedo, Freighter, Rabet and xBull) already support sending the network as a parameter, that could be an option.

For example in my kit I have the function:

signTx(params: { xdr: string; publicKeys: string[]; network: WalletNetwork }): Promise<{ result: string }>;

This way the dapp can always tell the wallet the exact network it needs to use

[piyalbasu]

The interface above was definitely a bit simplified to kickoff the discussion. IMO, an actual interface would look more like:

{
  getPublicKey: () => /* return value up for debate */,
  signTransaction(xdr: string, opts?: { network?: string, networkPassphrase?: string, accountToSign?: string }) =>  /* return value up for debate */,
  signAuthEntry(xdr: string, opts?: { network?: string, networkPassphrase?: string, accountToSign?: string }) => /* return value up for debate */,
  getNetwork() => {"network": string, "networkUrl": string, "networkPassphrase": string, "sorobanRpcUrl": string} ,
}

[namankumar]

We could draw some inspiration from Ethereum as well.

EIP-1193 was introduced to standardize a dapp connecting with different wallets from different providers - which is something I believe this proposal is trying to achieve as well

EIP-6963 was introduced to let a dapp discover when a browser wallet was installed, which becomes important when a user has multiple wallets installed

https://medium.com/walletconnect/goodbye-browser-extension-wallet-wars-eip-6963-is-now-approved-3fe0bc7476c8

[janewang]

@piyalbasu I'd like to suggest some interface additions. It could be for next iteration of this SEP.

connect()
disconnect()
accountsChanged() - When the user changes to another account 
sendTransaction(xdr: string) - This is the case where dapp relies on the wallet to send the transaction onward to rpc, instead of letting the dapp to interact with an RPC (on mainnet a lightweight dapp has to register separately to an RPC, instead of relying on a wallet to send the tx)

[orbitlens]

Methods like connect(), disconnect(), and accountsChanged() can be easily implemented on the client app side, with greater flexibility.

Regarding sendTransaction(), we can probably introduce additional submit: boolean parameter in the signTransaction() method. That's what we do currently in Albedo.

[piyalbasu]

I've incorporated changes we've agreed to here as well as those I've collected on Discord on this PR: #1478

[earrietadev]

Just wanted bring something else to the table that it might be good to include: a way to import an asset.

Since we are promoting the usage of assets list, something I have used a lot when using EVM wallets is clicking on a protocol's site and add the asset to my wallet (basically it adds the asset contract ID). This is good for users that might be using a protocol that is not that popular yet or for custom assets that will probably be outside of the lists (like pool shares)

[janewang]

I'd like to suggest an importAccountByPrivateKey method to be implemented by all wallets.

Occasionally wallets shutdown for various reasons (no more run way, key member departure, so on and so forth). Users could migrate off the wallet by using seedphrase, but in all instances, this is too challenging for all the users. If wallets consistently have the ability to import account by private key, the wallet about to shutdown can push a feature to export keys and import to other wallets. It makes migration off a wallet less choatic. In general be able to import by key is useful from dev perspective too.

[willemneal]

Usually wallets use seed phrases. This helps users create multiple wallets and is much easier to export/import.

[piyalbasu]

This is actually something some others in the ecosystem have requested Freighter implement. We've decided not to implement this as we have felt we want to discourage users from copy/pasting private keys.

cc: @brunomuler

[earrietadev]

I think there is value in making an standard for migrating form one wallet to another one, in xBull I already allow that by exporting an encrypted file, later I can import this to any xBull (mobile, web and extension) when I need to. If all wallets accept something similar I think we could make it easy for users to migrate if they need to and it can be done without them needing to copy/paste keys nor phrases

[janewang]

Yes, the key should be encrypted using a password during transfer. Alternatively, we can consider sweep an account. The point is aligning on something consistent such that it makes it easier to migrate users.

[piyalbasu]

I like the idea of an encrypted account migration. I think that it may be a bit out of scope for this standard, which focuses on exposing an API - maybe we should consider this for a different standard as I do think this is definitely important functionality to standardize!

I'd mostly be concerned about exposing this method to dapps, which could lead to scams. I'd rather this functionality be contained to the wallet where only the wallet owner has access to it

[mtwtfss]

@earrietadev and @orbitlens we're planning to finalize this proposed standard during the Stellar Protocol Meeting next week on Thursday, June 6th at 1:00 PM PDT. We'd love your support and participation in the meeting.

[namankumar]

The corresponding proposal was put to vote on 6/6/24. The votes represent ecosystem sentiment and can be used by the author to determine next steps

The voting card is on testnet deployment of Soroban Governor (link)

Here is the snapshot of the final count:

[namankumar]

I'm curious which wallets are planning to implement or have already implemented this standard? @piyalbasu @earrietadev @orbitlens

[earrietadev]

This interface is included in the Stellar Wallets Kit, xBull already supports part of the interface but not all of the methods yet

[orbitlens]

Do we really need signAuthEntry method? What's the case for it?

Someone mentioned earlier that it can be useful for multisig. However, to my mind, signTransaction must handle all multisig aggregations instead. This way developers don't need to think which method they need to utilize for a particular transaction and how the multisig or multi-authorization should be handled.

[piyalbasu]

signAuthEntry is mostly used for atomic swaps on Soroban. A user needs to sign just for the authentication of their part of the swap, not the whole transaction. We had talked internally about creating a generic signing method where a caller doesn't need to choose the type of payload to sign, but it seems as though this may not be possible: #1467 (comment)

Because of that, we opted for a separate signAuthEntry method

[orbitlens]

How getAddress() method should behave if the wallet contains more than one account and a user previously interacted with this site using several different addresses? Should it be the last utilized address?

Do we want to make sure that a user really owns the account he authenticating with? I mean, someone can easily clone the open-source wallet repo, and slightly change the code to impersonate some other account owner, returning a public key from the account they doesn't own (e.g., one of SDF's accounts). We have two options: either sign and return some challenge in this method, similar to the signMessage() method, or simply mention in SEP that getAddress() doesn't guarantee the account ownership and if developers need to authenticate an account owner, they should use signMessage() method for this purpose.

[earrietadev]

IMO getAddress should just return the selected account on the wallet and about the impersonation part, the app should be aware of that and be the one that takes care of making sure an account is actually owned by the user (for example using SEP-10 if the app needs to show something other accounts should't see)

[orbitlens]

We probably need to standardize the implementation of the signMessage() method.

Our algorithm:

function signMessage(message, keypair) {
  const publicKey = keypair.publicKey()
  const signedMessage = keypair.publicKey()+':'+message //protection from spoofing
  const messageHash = sha256(signedMessage)
  const signature = keypair.sign(messageHash).toString('hex')
  return {publicKey, signature, signedMessage}
}

From my point of view, the networkPassphrase argument is irrelevant here since it's a general-purpose method, not tied to a specific Stellar network. If developers want to make sure that the user is signing message for a specific network, they can include network identifier into the message argument.

[overcat]

We should ensure that any string (Chinese, Japanese, etc.) can be properly processed. Also, I think using Stellar Signed Message:\n' as a prefix could be another option, as this format is widely adopted. Here's a possible implementation:

function signMessage(message, keypair) {
    const signedMessage = 'Stellar Signed Message:\n' + message;
    const messageBuffer = Buffer.from(signedMessage, 'utf8');
    
    const messageHash = sha256(messageBuffer);
    
    const signature = keypair.sign(messageHash);
    const publicKey = keypair.publicKey();
    
    return {
        publicKey,
        signature,
        signedMessage
    };
}

[orbitlens]

Regarding the getNetwork() method. We don't support this method currently because it can potentially cause weird situations when a user temporarily switched to the Testnet and after that can't sign a transaction in a dapp interface that works with the Pubnet.

As I see it, the network selection during the transaction signing should be controlled by the dapp UI. A user should see the requested network in the wallet interface during the request authorization, but shouldn't be able to change it (again, to avoid weird glitches). The network, selected by the user in the wallet interface prior to the transaction signing request must not affect the request received from the dapp. If the network was not specified in the request, the wallet should assume that the dapp uses Stellar Pubnet by default.

[piyalbasu]

Do you think there's a security implication here? For example, a dapp could claim that their transaction is only meant for testnet, but then actually configure for mainnet. The user would have the opportunity to see the discrepancy in the wallet's UI, but they might miss it if they're not careful