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

Update caip-25.md #5

Closed
wants to merge 1 commit into from
Closed

Update caip-25.md #5

wants to merge 1 commit into from

Conversation

pedrouid
Copy link
Owner

@pedrouid pedrouid commented May 9, 2022

No description provided.

Comment on lines +68 to +74
"result": [
{
"accounts": ["eip155:1:0xab16a96d359ec26a11e2c2b3d8f8b8942d5bfcdb"],
"methods": ["eth_sendTransaction", "eth_signTransaction", "eth_sign", "personal_sign"]
"events": ["accountsChanged", "chainChanged"]
}
]
Copy link

@rekmarks rekmarks May 10, 2022

Choose a reason for hiding this comment

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

I think this format makes sense! Some questions / observations:

  1. The wallet can return an arbitrary number of result objects in the array with any combination of accounts, methods, and events, right? I assume that's your intention here, I just want to confirm.
  2. This response format only makes sense for methods and events that parameterize the account(s) and chain(s) in some way. However, in some cases only the chain is parameterized, and for other cases neither the account nor the chain are parameterized. Could we permit results of this form?
    "result": [
        // Account and chain parameterized
        {
            "accounts": ["eip155:1:0xab16a96d359ec26a11e2c2b3d8f8b8942d5bfcdb"],
            "methods": ["eth_sendTransaction", "eth_signTransaction", "eth_sign", "personal_sign"],
            "events": ["accountsChanged", "chainChanged"]
        },
        // Only the chain parameterized
        {
            "chains": ["eip155:10"],
            "methods": ["eth_blockNumber"],
            "events": ["chainChanged"]
        },
        // Neither account nor chain parameterized
        {
            "methods": ["wallet_installSnap", "wallet_getPermissions", "wallet_requestPermissions"],
            "events": ["wallet_locked", "wallet_unlocked"]
        },
    ]
  1. The observation in (2) has implications for the params in the handshake request as well, specifically if the application requests methods for which the chain is not parameterized, like wallet_getPermissions. For that reason, could we permit omitting the chains property from the handshake param objects as well?
  2. You'll note my inclusion of wallet_ methods here. Would they be a separate namespace in CAIP terms or how will that work?
  3. Finally, a non-blocking observation: this looks like it would be annoying to parse in practice. Personally, I would want to store this response data in some kind of map, so I'm wondering if we could figure out a way to return a single object keyed by namespace. That said, there probably isn't a one-size-fits-all solution, so application devs should be able to live with this as long as there's good tooling to go along with it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks @rekmarks for the feedback!

I definitely agree that we consider a significantly smaller scope for this API that would involve essentially what would look like a Wallet API but I do like that you address several methods and events which aren't scoped within any particular chain but the wallet as a whole

We are going to discuss internally with the WalletConnect team regarding these points you raised but my first opinion would be that reducing the scope would definitely be the more valuable and potentially we could define schemas to extend these "permissions"

As you mentioned the proposed schemas would be insane to parse and a map would be substantially easier to parse hence we would need to consider how nesting between shared methods/events would work and additionally how would shared accounts/chains would be defined.

Our goal was definitely to simplify exposure of accounts as a reference to chains in their prefix which IMO would also cover scopes that parameterized by chains

Definitely lots of food for thought...

Choose a reason for hiding this comment

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

to +74
"result": [
{
"accounts": ["eip155:1:0xab16a96d359ec26a11e2c2b3d8f8b8942d5bfcdb"],
"methods": ["eth_sendTransaction", "eth_signTransaction", "eth_sign", "personal_sign"]
"events": ["accountsChanged", "chainChanged"]
}
]

@pedrouid
Copy link
Owner Author

Hey @rekmarks, after several internal debates about scope and cost/benefit for our namespaces implementations I believe that we have come to a very productive schema that is both pragmatic and extendable to tackle several requirements you described above.

Note that schemas below are describing CAIP-25 and may not directly correlate to interfaces defined by WalletConnect 2.0 protocol but still maintain compatibility.

The first interface defines what we believe will be the majority of use-cases and provides the most utility.

interface CAIP25RequestParams {
    [namespace]: {
        chains: string[], // caip-2 chainIds
        methods: string[], // json-rpc methods
        events: string[] // event names
    }
}

interface CAIP25ResponseResult {
    [namespace]: {
        accounts: string[], // caip-10 accountIds
        methods: string[], // json-rpc methods
        events: string[] // event names
    }
}

As you can see above we maintained the requirement for accounts to be parameterized rather than chains because they both contain the same information and more.

If you need events or methods that don't require an account then you can extract the unique prefixes from an accounts array. See the following example:

accounts = [
    "eip155:1:0xf7956411547cE70Ca15Ab2acd2AbFA5C3C32c0D8",
    "eip155:1:0x58315c689b483a66b951EDEfAbf8847c75e44B45",
    "eip155:10:0xf7956411547cE70Ca15Ab2acd2AbFA5C3C32c0D8",
    "eip155:137:0xf7956411547cE70Ca15Ab2acd2AbFA5C3C32c0D8",
]

chains = parseChains(accounts)
// ["eip155:1", "eip155:10", "eip155:137"]

Additionally the interface for CAIP-27 for JSON-RPC requests uses a chainId anyway regardless if accounts are parameterized

Finally you can see that created a map indexed by namespaces such as eip155 or cosmos to make parsing more efficient. This originally proposed by @Talhaali00 in the original PR in ChainAgnostic#108.

The reason I rejected this originally was because not all blockchains within a single namespace have the same methods and events and this would constrain all chains to share them.

So instead we added the concept of "extension" definitions which allow nested methods or events to be defined within a namespace for only a subset of chains.

Let's look at an example:

{
  "eip155": {
    "accounts": [
        "eip155:1:0x1bd5269795cfc4c8713ee13ce3fe86113f71cd63", 
        "eip155:321:0x1bd5269795cfc4c8713ee13ce3fe86113f71cd63
    ],
    "methods": ["eth_sendTransaction", "eth_signMessage"]
    "events": ["accountsChanged", "chainChanged"],
    "extension" : [
      {
        "accounts": ["eip155:321:0x1bd5269795cfc4c8713ee13ce3fe86113f71cd63"],
        "methods" : ["eth_signZKProof"]
      }
    ]      
  }
}

We have a single namespace with two accounts in two different chains sharing the common methods like sending transactions and signing messages. However the extension describes that the chain `eip155:321` has an extra method that allows an account to sign a ZK proof. This way we don't have to duplicate the description of methods and events in two separate namespaces and we can still support broader implementations of blockchain environments such as zkEVM. 

(please note that eth_signZKProof is hypothetical and so is chainId 321)

So this we actually expand the CAIP-25 interface for params and result as follows:
```typescript
interface CAIP25RequestParams {
    [namespace]: {
        chains: string[], // caip-2 chainIds
        methods: string[], // json-rpc methods
        events: string[] // event names
        extension?: RequestExtension[]
    }
}

interface CAIP25ResponseResult {
    [namespace]: {
        accounts: string[], // caip-10 accountIds
        methods: string[], // json-rpc methods
        events: string[] // event names
        extension?: ResultExtension[]
    }
}

interface RequestExtension {
    chains: string[],
    methods?: string[], // json-rpc methods
    events?: string[] // event names
}

interface ResultExtension {
    accounts: string[],
    methods?: string[], // json-rpc methods
    events?: string[] // event names
}

We have worked with a few designs over the past 9 months with WalletConnect 2.0 beta and we believe this would be not only hugely beneficial for multi-chain support but also expandable for future blockchains.

Our goal is to be pragmatic and we believe that this will meet your requirements for Metamask as well including methods that are wallet_ prefixed which can be scoped within the EIP155 namespace.

I would love to know what you think and please share your thoughts on our proposal!

@Talhaali00
Copy link

@pedrouid RequestExtension and ResultExtension both have optional methods and events. These properties should not be optional

@ritave
Copy link

ritave commented Jun 23, 2022

@pedrouid During Metamask Snaps research I came onto a need of feature discovery. For example, a DApp can request multiple namespaces, but the Wallet can only support some of them. I think the resulting CAIP25ResponseResult should be able to have less namespaces / chains compared to ones requested.

@pedrouid pedrouid closed this Apr 7, 2023
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.

5 participants