feat!: ADR-015: Improve chain link signature support #968

RiccardoM · 2022-07-18T05:33:59Z

Description

This PR adds support for different signature algorithms (namely EVM-compatible personal_sign) when creating a chain link, as per ADR-015.

personal_sign has been chosen to be the first EVM-compatible signature algorithm supported due to the fact that most wallets support this and it allows to specify an arbitrary message to be signed.

Closes: #871

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

codecov · 2022-07-18T08:01:16Z

Codecov Report

Merging #968 (3bcbc99) into master (c7ff50d) will decrease coverage by 0.05%.
The diff coverage is 76.25%.

❗ Current head 3bcbc99 differs from pull request most recent head c9faac3. Consider uploading reports for the commit c9faac3 to get more accurate results

@@            Coverage Diff             @@
##           master     #968      +/-   ##
==========================================
- Coverage   80.54%   80.49%   -0.06%     
==========================================
  Files         172      172              
  Lines       15095    15223     +128     
==========================================
+ Hits        12158    12253      +95     
- Misses       2415     2442      +27     
- Partials      522      528       +6

Impacted Files	Coverage Δ
x/profiles/types/models_chain_links.go	`74.28% <73.77%> (-2.86%)`	⬇️
x/profiles/legacy/v6/store.go	`67.55% <84.61%> (+4.44%)`	⬆️
x/profiles/types/codec.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7ff50d...c9faac3. Read the comment docs.

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

RiccardoM · 2022-07-18T11:41:15Z

@dadamu @bragaz If you can review this it would be great. I will take care of updating the docs once everything is fine for you

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

…/adr-015-improve-chain-link-signature-support

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

leobragaz

Awesome work! So this supports also the signatures made with Metamask right?

RiccardoM · 2022-07-19T13:14:24Z

Awesome work! So this supports also the signatures made with Metamask right?

Yes, it does. It supports ETH personal_sign signature. The test case present is actually testing against a signature generated with MetaMask

RiccardoM · 2022-07-20T07:02:34Z

proto/desmos/profiles/v3/models_chain_links.proto

+// --------------------------------------------------------------------------------------------------------------------
+
+// CosmosSingleSignature is the signature data for a single signer
+message CosmosSingleSignature {


I'm thinking if there are better ways to name this type after what @dadamu said:

Luckily, thanks to solana-wallet-adapter providing the function signing a raw message(arbitrary string), we can use the same process as CosmosSingleSignature [...].

This means that what has been currently named CosmosSingleSignature is actually compatible with other ecosystems different from the Cosmos one. For this reason, I think a better name should be used instead.

@dadamu Proposed GeneralSingleSignature, but maybe even a shorter SingleSignature can work.

What do you think @bragaz @kwunyeung?

Note
By looking at the code, we know that the signing algorithm used to verify the signature will be based on the type of the public key provided. The currently supported public key types are Ed25519, Secp256r1 and Secp256k1. As @dadamu said, for Solana we should use /cosmos.crypto.ed25519.PubKey (hence why this signature also works for that ecosystem).

@RiccardoM I think we can use something like: StandardSingleSignature or GeneralSingleSignature is ok. Then we can add in the docs the supported chains of such signature.

@bragaz Hmm I'm not sure about those. I mean what does Standard mean? Standard compared to what? Is there really a standard among all signatures?

The same for General: I don't see why we should prepend General and not just leaving SingleSignature instead

@RiccardoM From my side, the General is compared to EVM or other unique spec signatures. Noting that SingleSignature is the general signature based on general private/public key signing should be enough as well.

I'm okay with SingleSignature too. I was trying to add more context with General and Standard but it actually make more confusion after I've read @RiccardoM comment.

I ended up renaming CosmosSingleSignature to SingleSignature, and removing EVMSignature in favor or SingleSignature as well. Now EVM-compatible signatures can be specified by simply using the proper SignatureValueEncoding inside a SingleSignature itself:

var evmSignature = types.NewSingleSignature( types.SIGNATURE_VALUE_ENCODING_EMV_PERSONAL_SIGN, signatureBz. )

Since the verification for both Cosmos and EVM signatures relies on the PubKey type anyway, there was no need to distinguish them aside from validating how the value was encoded. But this can be done by simply using an enum of possible value encoding algorithms supported. This should make it easier for clients and developers to create the correct signature type since we only have two possible types (SingleSignature and CosmosMultiSignature).

dadamu · 2022-07-20T11:16:42Z

x/profiles/legacy/v6/store.go

+ panic(fmt.Sprintf("unsupported signing mode: %s", signature.Mode))
+ }
+
+ v6Signature := types.NewCosmosSingleSignature(signingMode, signature.Signature)
 signatureAny, err = codectypes.NewAnyWithValue(v6Signature)
 if err != nil {
 panic(err)
 }

 case *v5types.MultiSignatureData:


Can you add a test for MultiSignatrueData migrations?

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

dadamu · 2022-07-21T07:23:01Z

proto/desmos/profiles/v3/models_chain_links.proto

- // Mode is the signing mode of the single signer
- cosmos.tx.signing.v1beta1.SignMode mode = 1;
+ // ValueEncoding represents how the value has been encoded before being signed
+ SignatureValueEncoding value_encoding = 1


I would remain the naming mode with SignMode instead of value_encoding with SignatureValueEncoding.

I thought about this too, but then I thought: signature modes are things like Secp256k1, Ed25519, etc while in this case we want to specify how the signature value has been encoded before signing (Amino, Protobuf, PersonalSign, etc). So I think this makes more sense. Does it make sense to you?

I think PersonalSign is not a encoding method since the plain text of it is raw text, it is the method to sign a raw text message into signature. Besides, secp256k1 and ed25519 algorithms are based on the keys since they use different curves to sign message with the key, it is described here.

Maybe we can just call it SignatureType then? And have SIGNATURE_TYPE_COSMOS_AMINO, SIGNATURE_TYPE_EVM_PERSONAL_SIGN, etc?

@RiccardoM That is great! I agree with it.

I ended up going for SignatureValueType which I think it's more explanatory than SignatureType which might be confused for something else

dadamu · 2022-07-21T08:35:14Z

x/profiles/types/models_chain_links.go

+type CosmosSignature interface {
+ Signature
+ GetSignatureValueEncoding() (SignatureValueEncoding, error)
+}


Shouldn't CosmosSignature merge into Signature and remove this interface?

Yeah it makes sense. Done that

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

dadamu

Awesome job! Ready to ship~

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

…ture-support

RiccardoM added 3 commits July 15, 2022 14:32

added the types definitions

641264d

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

updated the types again

5f4420e

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

fixed unit tests

76c0a6c

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

github-actions bot added kind/build Related to the build of the project x/CLI x/profiles Module that allows to create and manage decentralized social profiles labels Jul 18, 2022

RiccardoM added 3 commits July 18, 2022 07:38

fixed Proto lint errors

1e6d09f

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

added norace flags to CLI tests

54ec48b

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

fix tests

bdfa7a5

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

updated tests to include EVM signature

b800270

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

RiccardoM requested review from dadamu and leobragaz July 18, 2022 11:40

RiccardoM marked this pull request as ready for review July 18, 2022 11:40

added changeset entry

b175e47

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

RiccardoM changed the title ~~feat: ADR-015: Improve chain link signature support~~ feat!: ADR-015: Improve chain link signature support Jul 18, 2022

RiccardoM added 3 commits July 19, 2022 14:37

Merge branch 'master' of github.com:desmos-labs/desmos into riccardom…

89ecc0e

…/adr-015-improve-chain-link-signature-support

updated deps

aec3536

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

fixed tests

5c4d146

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

leobragaz reviewed Jul 19, 2022

View reviewed changes

leobragaz approved these changes Jul 19, 2022

View reviewed changes

RiccardoM commented Jul 20, 2022

View reviewed changes

dadamu reviewed Jul 20, 2022

View reviewed changes

RiccardoM added 4 commits July 21, 2022 07:44

renamed CosmosSingleSignature to SingleSignature

4809e55

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

removed EVMSignature in favor of SingleSignature

a724610

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

updated golint configuration

cda49bd

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

updated ADR

11c54ab

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

github-actions bot added the kind/adr An issue or PR relating to an architectural decision record label Jul 21, 2022

github-actions bot added the kind/ci Improve the CI/CD label Jul 21, 2022

reverted lint configuration

414cece

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

github-actions bot removed the kind/ci Improve the CI/CD label Jul 21, 2022

added multisig migration test

cde1f35

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

RiccardoM requested a review from dadamu July 21, 2022 06:37

dadamu reviewed Jul 21, 2022

View reviewed changes

leobragaz approved these changes Jul 21, 2022

View reviewed changes

dadamu reviewed Jul 21, 2022

View reviewed changes

RiccardoM added 2 commits July 21, 2022 10:41

renamed SignatureValueEncoding to SignatureValueType

5cd05d5

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

updated test methods names

3bcbc99

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

RiccardoM requested a review from dadamu July 21, 2022 08:54

dadamu approved these changes Jul 21, 2022

View reviewed changes

updated the spec

54acb6b

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

RiccardoM added the automerge Automatically merge PR once all prerequisites pass label Jul 21, 2022

Merge branch 'master' into riccardom/adr-015-improve-chain-link-signa…

c9faac3

…ture-support

mergify bot merged commit 058447a into master Jul 21, 2022

mergify bot deleted the riccardom/adr-015-improve-chain-link-signature-support branch July 21, 2022 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: ADR-015: Improve chain link signature support #968

feat!: ADR-015: Improve chain link signature support #968

RiccardoM commented Jul 18, 2022 •

edited

Loading

codecov bot commented Jul 18, 2022 •

edited

Loading

RiccardoM commented Jul 18, 2022

leobragaz left a comment

RiccardoM commented Jul 19, 2022 •

edited

Loading

RiccardoM Jul 20, 2022 •

edited

Loading

leobragaz Jul 20, 2022 •

edited

Loading

RiccardoM Jul 20, 2022

dadamu Jul 20, 2022

leobragaz Jul 20, 2022

RiccardoM Jul 21, 2022 •

edited

Loading

dadamu Jul 20, 2022

RiccardoM Jul 21, 2022

dadamu Jul 21, 2022

RiccardoM Jul 21, 2022

dadamu Jul 21, 2022 •

edited

Loading

RiccardoM Jul 21, 2022

dadamu Jul 21, 2022

RiccardoM Jul 21, 2022 •

edited

Loading

dadamu Jul 21, 2022 •

edited

Loading

RiccardoM Jul 21, 2022

dadamu left a comment

feat!: ADR-015: Improve chain link signature support #968

feat!: ADR-015: Improve chain link signature support #968

Conversation

RiccardoM commented Jul 18, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Jul 18, 2022 • edited Loading

Codecov Report

RiccardoM commented Jul 18, 2022

leobragaz left a comment

Choose a reason for hiding this comment

RiccardoM commented Jul 19, 2022 • edited Loading

RiccardoM Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

leobragaz Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RiccardoM Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadamu Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RiccardoM Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

dadamu Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadamu left a comment

Choose a reason for hiding this comment

RiccardoM commented Jul 18, 2022 •

edited

Loading

codecov bot commented Jul 18, 2022 •

edited

Loading

RiccardoM commented Jul 19, 2022 •

edited

Loading

RiccardoM Jul 20, 2022 •

edited

Loading

leobragaz Jul 20, 2022 •

edited

Loading

RiccardoM Jul 21, 2022 •

edited

Loading

dadamu Jul 21, 2022 •

edited

Loading

RiccardoM Jul 21, 2022 •

edited

Loading

dadamu Jul 21, 2022 •

edited

Loading