Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SIP-030: Wallet RPC Standards #166

Open
wants to merge 46 commits into
base: main
Choose a base branch
from
Open

SIP-030: Wallet RPC Standards #166

wants to merge 46 commits into from

Conversation

janniks
Copy link
Contributor

@janniks janniks commented Jan 12, 2024

Wallet RPC Standards

📓 Read the full SIP-030 on Wallet RPC Standards

  • Adds SIP for more modern standard for wallet API RPC (usable for browser-extensions as well as other settings)
  • Proposes standards for serialization of wallet RPC requests

Currently being reviewed and edited for final approval

@kyranjamie
Copy link

Great work @janniks. Leather supports this SIP 🖤

@janniks
Copy link
Contributor Author

janniks commented Feb 14, 2024

Thanks for the input @m-aboelenein @kyranjamie .
I updated the stuff we chatted about on the call, and also added some OPEN QUESTIONS to the top.

Do you think you could do another review with final remarks until ~Tue. 20.02.2024 to wrap this up with bipartisan support? 🙏

@janniks
Copy link
Contributor Author

janniks commented Feb 15, 2024

Added methods getAddresses and getAccounts for better compatibility with current solution.

@janniks
Copy link
Contributor Author

janniks commented Feb 29, 2024

@aryzing @kyranjamie I added an update to the encoding (as discussed on last weeks call).
Let me know what you think! This can be used as the canonical repr and Stacks.js would also migrate towards it. (Serialized hex encoded string could also still be used (easy to tell apart).

Also updated:

  • renamed functionName to name, functionArgs to arguments
  • merged contractName + contractAddress to contract -- which is a ${string}.${string} (how it's copy pasted from the explorer)
  • renamed some other params (see 425f81c for details)

Ping me if you disagree with anything -- trying to make the naming more logical and short

@janniks
Copy link
Contributor Author

janniks commented Feb 29, 2024

CC @pradel would also love to get your feedback on this updated approach to "Connect"/auth -- it's less convoluted, but also more explicit (might need multiple steps for some things, that were just magically done in auth previously , e.g. requesting a gaia token would be it's own step, (although wallets could also add it to something like getAddresses / getAccounts)

@pradel
Copy link

pradel commented Mar 1, 2024

@janniks I like this change, less magic on what's going on and the behavior seems to be pretty similar to how ETH is doing things, making it easier for ETH devs to use the API

@janniks janniks changed the title SIP: Wallet APIs via WebBTC SIP: Modern Wallet APIs Mar 5, 2024
@janniks
Copy link
Contributor Author

janniks commented Mar 5, 2024

CC @friedger would also love to get some of your feedback+expertise, since you've worked on this area in the past 🙏

@janniks
Copy link
Contributor Author

janniks commented Mar 5, 2024

Also, Thanks for all the feedback everyone! 👏

I feel like we're nearing a good document here. I think we can wrap it up soon 🫡

@janniks
Copy link
Contributor Author

janniks commented Mar 5, 2024

Let me know which email-addresses/github-usernames to add to the author list. cc @kyranjamie @m-aboelenein @aryzing

sips/sip-030/sip-030-wallet-interface.md Outdated Show resolved Hide resolved
sips/sip-030/sip-030-wallet-interface.md Outdated Show resolved Hide resolved
Wallets implementing the new standard may maintain the previous system to support legacy applications during a transition period or indefinitely.
Existing applications using the current Auth system should continue to operate, but immediate changes are recommended once this SIP is ratified.

## Reference Implementations
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reference implementations are the one of leather and xverse, aren't they?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if there is one, i think it would be helpful to share that information here rather than solely describing how the implementation should work.
is there an existing implementation of this proposal?

i would argue that most of this section should be moved to the Specifications section, since that's what they appear to be defining: specifications, not implementation details.

- Addresses are serialized as Stacks c32-encoded strings.
- Clarity values, post-conditions, and transactions are serialized to bytes (defined by SIP-005) and used as hex-encoded strings.

### Methods
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this part of the specification?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 - made a comment above about this exact thing before seeing you state the same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method definitions? They are sort of the largest part of the spec. But we can add more on specs for provider discovery and implementation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes - i think it's more accurate to have them in the specifications section rather than the implementation section.
the current text doesn't describe or define an implementation of the specification is my reasoning.

i referred to https://github.com/stacksgov/sips/blob/main/sips/sip-000/sip-000-stacks-improvement-proposal-process.md?plain=1#L165-L170 for my reasoning here.

sips/sip-030/sip-030-wallet-interface.md Outdated Show resolved Hide resolved
sips/sip-030/sip-030-wallet-interface.md Outdated Show resolved Hide resolved

### Provider registration

Wallets can register their aliased provider objects however they see fit.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this specific enough? Currently, different wallets use different methods and are not working nicely together.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree we should add more on provider discovery.


#### Event `accountChange`

`listener: (accounts: {}[]) => void`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if this is a privacy issue. A user might only wish to share one particular account details with a given app, but this switcher api implicitly shares all account details, (if a user switches to all accounts).

Instead, perhaps we should return only the account index, such that the app can know the account has changed.

@314159265359879
Copy link
Contributor

I liked the previous title of this standard better because it was easier for a lay person to understand.

What do you think about one of these alternatives instead?

  1. Unified Wallet Communication Standards
    (Highlights the goal of creating a single standard for wallet communication without technical jargon.)
  2. A Modern Standard for Stacks Wallet Connections
    (Conveys the update and relevance of the proposal.)
  3. Simplifying Wallet Interactions Across Platforms in the Stacks Network
    (Focuses on the user-friendly impact of the standard.)

Comment on lines +208 to +216
#### Method `stx_getAddresses`

> **Note**: This method can be used similarly to legacy "connect" methods, where the first account selection also acts as an approval of the site/domain.

Result properties

- `addresses`: `{}[]`
- `address`: `string` address, Stacks c32-encoded
- `publicKey`: `string` hex-encoded

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional network param here?

- `txid`: `string` hex-encoded
- `transaction`: `string` hex-encoded raw transaction

#### Method `stx_transferSip9Nft`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stx_transferSip13Sft could be added as well.


Errors thrown by request methods should match existing JSON-RPC 2.0 error codes.
This way, the user or an intermediary library can handle them in a standardized way.
Otherwise, no additional error codes are defined in this SIP.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Common errors like "user denied request" should have a defined error code
Leather uses 4001.


SIP Number: `030`

Title: Integration of a Modern Stacks Wallet Interface Standard

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Title: Integration of a Modern Stacks Wallet Interface Standard
Title: Definition of a Modern Stacks Wallet Interface Standard

Comment on lines +178 to +182
#### Method `stx_signTransaction`

Parameter properties

- `transaction`: `string` hex-encoded raw transaction

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#### Method `stx_signTransaction`
Parameter properties
- `transaction`: `string` hex-encoded raw transaction
#### Method `stx_signTransaction`
Parameter properties
- `transaction`: `string` hex-encoded raw transaction
- `broadcast?`: `boolean` whether transaction is to be broadcast, defaults to false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.