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

Implementer Feedback: CAIP-122<>EIP-4361 mismatch #264

Open
bumblefudge opened this issue Feb 20, 2024 · 12 comments
Open

Implementer Feedback: CAIP-122<>EIP-4361 mismatch #264

bumblefudge opened this issue Feb 20, 2024 · 12 comments

Comments

@bumblefudge
Copy link
Collaborator

resources is optional in EIP-4361 and required in CAIP-122 (although nothing stops it from being present but empty). both are still in review, but weight-bearing. presumably the path of least resistance/breakage is to keep it as-is, but add non-normative guidance warning people about this possibility (i.e. make "present but empty" explicitly allowed by 4361 to make sure implementers cover that case).

@bumblefudge
Copy link
Collaborator Author

As of now, just checked some stuff, some kind of PR on CAIP-122 is probably necessary. pinging @ukstv to see if the message format in #236 was implemented by ceramic, not seeing it reflected here which makes me wonder if we need an alignment [meeting]

property EIP CAIP in CAIP spec examples?
scheme opt n/a no
domain req req yes
address req req yes
statement opt opt yes
uri req req yes
version req req yes
chain-id req n/a - see PR#236 no
nonce req optional yes
issued-at req optional yes
expiration-time opt opt no
not-before opt opt no
request-id opt opt no
resources opt opt yes

@jdsika
Copy link

jdsika commented Feb 28, 2024

I will link this here as your table also mentions the attribute "address": #266

@jdsika
Copy link

jdsika commented Feb 28, 2024

I think #236 does not make much sense as the CAIP-10 clearly defines an account ID including the namespace and chain ID as prefix. This makes the chain ID obsolete and I think the account ID is a good solution and well readable

@jdsika
Copy link

jdsika commented Feb 28, 2024

Maybe I am just confused why it is split up....? And why is it not also:
blockchain == ${namespace(address)}

@bumblefudge
Copy link
Collaborator Author

bumblefudge commented Mar 22, 2024

@oed @haardikk21 @chris13524 you guys have opinions on reconciling these? i'm happy to open a PR and get a review from @jdsika off the top of my head but I've never implemented SIWX and have no idea what i'd be breaking!

@obstropolos
Copy link
Contributor

@oed @haardikk21 you guys have opinions on reconciling these? i'm happy to open a PR and get a review from @jdsika off the top of my head but I've never implemented SIWX and have no idea what i'd be breaking!

Also paging @0xproflupin for any thoughts here as well as it relates to SIWS compatibility.

@jdsika
Copy link

jdsika commented Mar 25, 2024

So I am the "fresh pair of eyes" on the whole thing but I may not know the backstory to some decisions. Suming up some things I think are actually inconsistencies:
#266

  • rename address to account-id
  • Styleguide: give conventions if snakecase camlcase etc should be used etc. CAIP-10: account_id and CAIP-122: account-id
  • replace blockchain with ${namespace(account-id)}

Important as I realized while doing tezos-caip-122:

  • Depending on the cryptographic curve and hashing function to derive the pkh from the public key it is NOT possible to get the public key from the pkh
  • In Ethereum it apparently is but in Tezos the wallet interaction TZIP defines a handshake to provide the public key from wallet to the application
  • Recommendation: Introduce a public-key in the CAIP-122 data model as optional and/or clearly define the need to provide this information to validate the signature
  • Give a recommendation to define the signature enums like $namespace as prefix + : + some curve specific suffix

@glitch-txs
Copy link

glitch-txs commented Apr 4, 2024

Hi guys, I found a couple of typos in the spec:

  • The Chain ID is not presented in the following table:
    image

  • The above table says the address must be CAIP-10 compliant but this is not shown in the Ethereum or Solana examples:
    image

  • I'm not familiar with Solana tbh, but I see it's example has Chain ID: 1 is this correct for Solana?


Joining a little bit the discussion here:

I think #236 does not make much sense as the CAIP-10 clearly defines an account ID including the namespace and chain ID as prefix. This makes the chain ID obsolete and I think the account ID is a good solution and well readable

This seems like a conflict to me between making the protocol "efficient" against avoiding deprecating EIP-4361, seems like there's intention to avoid breaking changes in Ethereum.
I think if we think of introducing breaking changes into EIP-4361 then would the following statement from the spec be correct?

This work is meant to generalize and abstract the Sign in With Ethereum specification, thereby making EIP-4361 a specific implementation of this specification, to work with all blockchains.

@bumblefudge
Copy link
Collaborator Author

bumblefudge commented Apr 4, 2024

This seems like a conflict to me between making the protocol "efficient" against avoiding deprecating EIP-4361, seems like there's intention to avoid breaking changes in Ethereum. I think if we think of introducing breaking changes into EIP-4361 then would the following statement from the spec be correct?

This work is meant to generalize and abstract the Sign in With Ethereum specification, thereby making EIP-4361 a specific implementation of this specification, to work with all blockchains.

Agreed, I added a commit wordsmithing this here - reapprove if that works for you?

I'm not familiar with Solana tbh, but I see it's example has Chain ID: 1 is this correct for Solana?

Technically, this would be a nonconformant message, but since the EIP-4361 examples and tutorials and SDKs out there in the wild all use chainId: 1 for mainnet, and the Solana environment doesn't have a native numeric chainId system (they organize networks more by stable RPC endpoints, as many L1s do), a Solana dev might be "forgiven", à la Postel's Law, for assuming that 1, and not 5eykt4UsFv8P8NJdTREpY1vzqKqZKvdpKuc147dw2N9d, is the CAIP-2 identifier for Solana mainnet. It's up to the implementation if they want to accept reasonable "aliases" for chainId or be strict in the Postel sense-- they're conformant with the spec either way. There is also an example of "aliases" in the recently-merged tezos CAIP-2 profile, on the subject of aliases. Note that the EIP 4361 original gives an ABNF for translating to user-readable text deterministically; showing an end-user a prompt that reads ChainId: 5eykt4UsFv8P8NJdTREpY1vzqKqZKvdpKuc147dw2N9d is a little cryptic, but the authors of EIP-4361 decided that the onus of translating chainIds to user-recognizable names was a UX extension EIP for a later day and the wallet's obligation/option in the meantime.

@glitch-txs
Copy link

I see, in the Solana case would the use of 5eykt4UsFv8P8NJdTREpY1vzqKqZKvdpKuc147dw2N9d be valid as a Chain ID parameter?

@bumblefudge
Copy link
Collaborator Author

valid yes; user-friendly, not so much. Designating official aliases could be done in a PR to solana/caip2.md in the namespaces project, if it had demonstrable support from the Solana community. Presumably "1" or "mainnet" would be the official alias?

@glitch-txs
Copy link

glitch-txs commented Apr 4, 2024

user-friendly, not so much.

In the case Chain ID was optional and CAIP-10 compliance required for address, devs would still be required to use 5eykt4UsFv8P8NJdTREpY1vzqKqZKvdpKuc147dw2N9d right?

Also "official aliases" seems to have a different description/meaning compared to "Chain ID"

Or do you mean to replace it in CAIP-2?

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

No branches or pull requests

4 participants