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

feat: Adds CAIP-122 standard for XRP Ledger. #84

Merged
merged 3 commits into from
May 23, 2024

Conversation

antondalgren
Copy link
Contributor

Aligned with the ripple-keypairs implementations for signing and verification.

* feat: Adds CAIP-122 standard for XRP Ledger.

* fix: Add some clarifications and follow the ripple-keypairs implementations for signing and verification

* fix: Branch was renamed from master to main

* fix: Remove double
@antondalgren antondalgren changed the title feat: Adds CAIP-122 standard for XRP Ledger. (#2) feat: Adds CAIP-122 standard for XRP Ledger. Aug 16, 2023
@antondalgren
Copy link
Contributor Author

Any news on this @bumblefudge?

@bumblefudge
Copy link
Collaborator

LGTM with minor edits, which i left on your fork. Are there already public and open source implementations of this that you can link to from the # References section? If so, please add in the form:

[name of implementation]: link to git repository
{...}

  • [name of implementation]: Sample implementation by {organization(s)} [, live at {url}]

@antondalgren
Copy link
Contributor Author

Thank you!

Currently there are no reference implementations for this @bumblefudge

```
### Signature Verification

Signature verification behaves similarly. We can use standard [secp256k1][] or[Ed25519][] signature verification.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that in the case of EVM, there are multiple signing methods (the spec mentioned eip155, aka personal_sign, classic metamask arbitrary unstructured text signing, and eip1271, aka smart contract wallet signing, for signatures produced by deployed smart contracts). is there only one signing method in Ripple, that you simply pass a secp256k1 or ed25519 key to, or are they two different signing interfaces with slightly different digest constraints/encodings, etc?

if the latter, it would be good to define the ENUM of valid signatureMeta.t values (recognizable names of signature interfaces in the XRP toolchain). and if the former, maybe just an ENUM with only one member? 😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://xrpl.org/signing-methods.html

I hadn't thought about multi-signatures, but maybe we just include that in the enum and let future teams worry about that one 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is only one signing method in Ripple, that you simply pass a secp256k1 or ed25519 key.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i mean, multi-signature might be the same algorithm/method but isn't the payload a little differently constructed? or am i missing something? you don't need to define it, I just want to make sure someone else defining it later understands how to do it (i.e., how to PR this adding a section for what's particular to it and adding a new valid value for SignatureMeta.t)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think multi-signature is relevant for this use case. But I'm happy to learn more about it otherwise.

It is used to verify the that the signature originates from the provided public key
SigningPubKey String
}
```

### Signature Creation

The abstract data model must be canonicalized to a string representation in an unambigious format, and then the string serialized to the proper binary format mentioned [above](#signing-algorithm) to be signed over with the proper algorithm.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it help to just specify JCS here, since it's the most general-purpose canonicalization for JSON?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@antondalgren @ukstv thoughts ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no strong opinions regarding adding it or not.

@@ -30,6 +30,16 @@ Correspondingly, a message needs to be serialized to bytes prior to signing it w

We propose using the signature type `xrpl:secp256k1` and `xrpl:ed25519` to refer to the chain and algorithm used uniquely.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would maybe add a sentence to this paragraph like,

These two signature types are the only two available and can be considered the defaults per key type. The typing of the signature MAY be made explicit by including a `SignatureMeta.t` property set to either `xrp1:secp256k1` or `xrp1:ed25519`, but if omitted the appropriate one of these two defaults should be obvious from the keytype provided in the `SignatureMeta.SigningPubKey` value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the context of your sentence is covered in the Signing Algorithm section.

@bumblefudge bumblefudge merged commit e39969c into ChainAgnostic:main May 23, 2024
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.

None yet

5 participants