feat: ADR-005: Support multisig chain link #708

dadamu · 2021-12-22T04:29:21Z

Description

Closes: #633
This PR implements ADR-005.

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)

dadamu · 2021-12-22T08:37:22Z

Hi, @RiccardoM @bragaz
It seems that we can not use SignatureDescriptor_Data in the Proof directly, The error happens when msg.GetSignBytes() is called, which is

--- FAIL: TestMsgLinkChainAccount_GetSignBytes (0.00s)
panic: Unregistered interface signing.isSignatureDescriptor_Data_Sum [recovered]
        panic: Unregistered interface signing.isSignatureDescriptor_Data_Sum

because signing.isSignatureDescriptor_Data_Sum is not registered to the AnimoCodec. In addition, the interface is a private interface generated by proto automatically so we can't register it into the codec in x/profiles.

I propose that we can convert it to hex string from the proto, instead.

What do you think?

RiccardoM · 2021-12-22T10:18:40Z

@dadamu What if we use the SignatureData interface instead, and encode it using an Any instance the same way we do with MsgLinkChainAccount#AddressData?

dadamu · 2021-12-22T14:01:41Z

@RiccardoM Unfortunately, SignatureData doesn't implement proto.Message. Besides, the structs of it are not the proto as well.
So it can not be used in the Proof proto object.

RiccardoM · 2022-01-05T09:57:44Z

@dadamu I have just finished experimenting with this problem, and I think that we can do as follows:

1. Create a new interface called SignatureData that extends proto.Message and has a isSignatureDataMethod
2. Create two new Proto definitions: SingleSignatureData and MultiSignatureData that contain the same fields of the Cosmos ones. The only difference is that our MultiSignatureData will have its Signatures field that is going to be an array of codectypes.Any rather than an array of signing.SignatureData.
3. Add a method called SignatureDataToCosmosSinatureData that takes a SignatureData interface instance and returns a signing.SignatureData instance.

Then, we only need to register the new SignatureData interface and its implementations inside our Amino instance and everything should work fine.

This will allow us to use proto.Any as the Signature type inside the Proof object, and serialize it both using Amino and Protobuf.

I have tried implementing all of this and pushed the changes to a new branch named riccardo/adr-005-multisig-chainlink-impl-test. You can take a look at it and pull the changes in your branch if you find them good enough

dadamu · 2022-01-05T10:15:27Z

@RiccardoM Okay, got it. will extend the interface with the same way as AddressData to solve the issue.

codecov · 2022-01-10T05:09:11Z

Codecov Report

Merging #708 (9eb1093) into master (2da9a43) will decrease coverage by 5.41%.
The diff coverage is 66.74%.

❗ Current head 9eb1093 differs from pull request most recent head 78be05b. Consider uploading reports for the commit 78be05b to get more accurate results

@@            Coverage Diff             @@
##           master     #708      +/-   ##
==========================================
- Coverage   84.23%   78.81%   -5.42%     
==========================================
  Files          91       53      -38     
  Lines        6624     4320    -2304     
==========================================
- Hits         5580     3405    -2175     
+ Misses        833      737      -96     
+ Partials      211      178      -33

Impacted Files	Coverage Δ
x/profiles/client/cli/cli_app_links.go	`34.16% <ø> (ø)`
x/profiles/client/cli/cli_params.go	`70.00% <ø> (ø)`
x/profiles/client/cli/cli_relationships.go	`75.51% <ø> (ø)`
x/profiles/client/utils/utils.go	`56.25% <ø> (ø)`
x/profiles/keeper/alias_functions.go	`90.47% <ø> (ø)`
x/profiles/keeper/genesis.go	`84.61% <ø> (ø)`
x/profiles/keeper/invariants.go	`100.00% <ø> (ø)`
x/profiles/keeper/keeper_blocks.go	`85.50% <ø> (ø)`
x/profiles/keeper/keeper_chain_links.go	`84.74% <ø> (ø)`
x/profiles/keeper/keeper_dtag_transfers.go	`89.09% <ø> (ø)`
... and 62 more

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 61ace0f...78be05b. Read the comment docs.

dadamu · 2022-01-12T04:27:17Z

x/profiles/legacy/v230/models_chain_links.go

@@ -0,0 +1,345 @@
+package v230


v230 is the copy paste of the latest chain link file for the v210 store_test.

dadamu · 2022-01-12T04:28:05Z

x/profiles/module.go

@@ -87,6 +88,7 @@ func (AppModuleBasic) GetQueryCmd() *cobra.Command {

 // RegisterInterfaces registers interfaces and implementations of the profiles module.
 func (AppModuleBasic) RegisterInterfaces(registry codectypes.InterfaceRegistry) {
+	v230.RegisterInterfaces(registry)


This is for v210 store_test as mentioned in v230 files.

app/desmos/cmd/chainlink/create_json.go

Co-authored-by: Leonardo Bragagnolo <leo.braga95@gmail.com>

…mos-labs/desmos into paul/adr-005-multisig-chainlink-impl

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

…mos-labs/desmos into paul/adr-005-multisig-chainlink-impl � Conflicts: � app/desmos/cmd/chainlink/create_json.go � app/desmos/cmd/chainlink/create_json_test.go

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

RiccardoM · 2022-01-12T11:24:16Z

@dadamu I've improved the code organization by splitting apart the various getters and creating a new builder interface. The multi-sig chain link test is however failing. Can you check why please? I didn't change pretty much anything except forcing the Sequence and AccountNumber to 0 when building the SignatureData instance

dadamu · 2022-01-13T04:38:51Z

@RiccardoM Fixed.
Also improve minor things. move provider folder outside of builder, and fix the multi provider to builder.

.changeset/entries/a492eff6c4ec45c98cb555b14de9f27ba50da5b147ff05b7a4cb1707af73655c.yaml

RiccardoM · 2022-01-13T07:57:56Z

app/desmos/cmd/chainlink/builder/multi/builder.go

+	}
+
+	// Get the sign mode
+	signMode := signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON


One thing I don't understand if it's correct or not is this line: the transaction could be signed using SIGN_MODE_DIRECT as well from what I know. Isn't this valid for multi-sig transactions as well? Or are they all signed using the SIGN_MODE_LEGACY_AMINO_JSON?

They are all signed in SIGN_MODE_LEGACY_AMINO_JSON if it is the multi-sig tx as well as the ledger tx.

Reference

@dadamu Perfect, thanks for the reference link as well! 🙏

…f05b7a4cb1707af73655c.yaml Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

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

…impl' into paul/adr-005-multisig-chainlink-impl

RiccardoM

I've just updated the create-chain-link-json documentation to include extensive explanation on what it's required when using it with multi-sig accounts. I've also added the missing docs to the generatePubKeyAndMultiSignatureData testing method.

I think the implementation is correct and ready to go.

leobragaz

Ready to ship

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 Dec 22, 2021

dadamu force-pushed the paul/adr-005-multisig-chainlink-impl branch from e2f37e7 to 7bb7535 Compare December 22, 2021 04:31

Apply SignatureData in Proof

b476e11

dadamu force-pushed the paul/adr-005-multisig-chainlink-impl branch from 7bb7535 to b476e11 Compare December 22, 2021 04:35

dadamu added 2 commits December 22, 2021 16:21

Fix types tests

11e307d

Add signature testutil

45b6567

changed how signature data should be provided inside the proof

b6df42d

dadamu added 10 commits January 6, 2022 16:43

Register interface

2fc266d

Fix panic in test

cdeb8a5

Remove signature nil checking since it causes panic

bef25df

Add back gogoproto.equal

73e6016

Update profiles types unit tests on multisig chainlink

73430a8

Update keeper unit tests

e05f47a

Update cli unit tests

4772a51

Update cli and v210 migration test

6b35fe3

Fix proof verify unpack issue

a9de5f0

Fix lint

3a84c6c

dadamu added 5 commits January 10, 2022 14:12

Add invalid signature test for Proof

7752995

Move SignatureDataFromCosmosSignatureData to types

144fac9

Build multisig chain link json cli

c82287e

Add unit test for chain link from multisign cli

1bc9de5

Remove unused config

9c6981e

dadamu commented Jan 12, 2022

View reviewed changes

Revert generateChainLinkJSON

b017cca

dadamu requested a review from leobragaz January 12, 2022 04:42

leobragaz reviewed Jan 12, 2022

View reviewed changes

app/desmos/cmd/chainlink/create_json.go Outdated Show resolved Hide resolved

dadamu and others added 4 commits January 12, 2022 16:50

Update app/desmos/cmd/chainlink/create_json.go

bd5bd67

Co-authored-by: Leonardo Bragagnolo <leo.braga95@gmail.com>

Fix the comment in cli test

2f0dae5

Merge branch 'paul/adr-005-multisig-chainlink-impl' of github.com:des…

65417f4

…mos-labs/desmos into paul/adr-005-multisig-chainlink-impl

Add test to check if signature is nil

a364e24

dadamu requested a review from leobragaz January 12, 2022 09:48

dadamu and others added 5 commits January 12, 2022 17:53

Remove unused function

31854ac

Add more tests for multisig proof

60deaa2

improved the overall code organization

41c7496

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

Merge branch 'paul/adr-005-multisig-chainlink-impl' of github.com:des…

40637f3

…mos-labs/desmos into paul/adr-005-multisig-chainlink-impl � Conflicts: � app/desmos/cmd/chainlink/create_json.go � app/desmos/cmd/chainlink/create_json_test.go

improved the overall code organization

1769abb

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

Fix sign mode and test builder

f198b21

Rerange the files

ef18023

RiccardoM reviewed Jan 13, 2022

View reviewed changes

Update .changeset/entries/a492eff6c4ec45c98cb555b14de9f27ba50da5b147f…

3f274d9

…f05b7a4cb1707af73655c.yaml Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

dadamu requested a review from RiccardoM January 14, 2022 06:18

RiccardoM added 2 commits January 14, 2022 09:19

small doc fixes

04b9181

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

Merge remote-tracking branch 'origin/paul/adr-005-multisig-chainlink-…

9eb1093

…impl' into paul/adr-005-multisig-chainlink-impl

RiccardoM approved these changes Jan 14, 2022

View reviewed changes

leobragaz approved these changes Jan 14, 2022

View reviewed changes

RiccardoM added the automerge Automatically merge PR once all prerequisites pass label Jan 14, 2022

Merge branch 'master' into paul/adr-005-multisig-chainlink-impl

78be05b

mergify bot merged commit 23cb923 into master Jan 14, 2022

mergify bot deleted the paul/adr-005-multisig-chainlink-impl branch January 14, 2022 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ADR-005: Support multisig chain link #708

feat: ADR-005: Support multisig chain link #708

dadamu commented Dec 22, 2021 •

edited

Loading

dadamu commented Dec 22, 2021 •

edited

Loading

RiccardoM commented Dec 22, 2021

dadamu commented Dec 22, 2021

RiccardoM commented Jan 5, 2022

dadamu commented Jan 5, 2022

codecov bot commented Jan 10, 2022 •

edited

Loading

dadamu Jan 12, 2022

dadamu Jan 12, 2022

RiccardoM commented Jan 12, 2022

dadamu commented Jan 13, 2022 •

edited

Loading

RiccardoM Jan 13, 2022

dadamu Jan 14, 2022

RiccardoM Jan 14, 2022

RiccardoM left a comment

leobragaz left a comment

feat: ADR-005: Support multisig chain link #708

feat: ADR-005: Support multisig chain link #708

Conversation

dadamu commented Dec 22, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

dadamu commented Dec 22, 2021 • edited Loading

RiccardoM commented Dec 22, 2021

dadamu commented Dec 22, 2021

RiccardoM commented Jan 5, 2022

dadamu commented Jan 5, 2022

codecov bot commented Jan 10, 2022 • edited Loading

Codecov Report

dadamu Jan 12, 2022

Choose a reason for hiding this comment

dadamu Jan 12, 2022

Choose a reason for hiding this comment

RiccardoM commented Jan 12, 2022

dadamu commented Jan 13, 2022 • edited Loading

RiccardoM Jan 13, 2022

Choose a reason for hiding this comment

dadamu Jan 14, 2022

Choose a reason for hiding this comment

RiccardoM Jan 14, 2022

Choose a reason for hiding this comment

RiccardoM left a comment

Choose a reason for hiding this comment

leobragaz left a comment

Choose a reason for hiding this comment

dadamu commented Dec 22, 2021 •

edited

Loading

dadamu commented Dec 22, 2021 •

edited

Loading

codecov bot commented Jan 10, 2022 •

edited

Loading

dadamu commented Jan 13, 2022 •

edited

Loading