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

Support multisig address inside MsgCreateChainLink #633

Closed
ryuash opened this issue Sep 29, 2021 · 5 comments · Fixed by #634 or #708
Closed

Support multisig address inside MsgCreateChainLink #633

ryuash opened this issue Sep 29, 2021 · 5 comments · Fixed by #634 or #708
Assignees
Labels
kind/new-feature Propose the addition of a new feature that does not yet exist x/profiles Module that allows to create and manage decentralized social profiles

Comments

@ryuash
Copy link
Contributor

ryuash commented Sep 29, 2021

Feature description

I would like to link my multisig addresses to my profile

Implementation proposal

@ryuash ryuash added the kind/new-feature Propose the addition of a new feature that does not yet exist label Sep 29, 2021
@dadamu
Copy link
Contributor

dadamu commented Sep 29, 2021

@RiccardoM @bragaz
The chain-link in Desmos also does not serve multisig account as well, the proto of chain-link should be modified to handle it.
In addition, create-chain-link-json cmd should be separated into create-json-file and sign-json-file as well.
What do you think?

@RiccardoM
Copy link
Contributor

RiccardoM commented Sep 29, 2021

@dadamu Can we have a specification of how it should be edited? It's probably better to open an ADR for this

@dadamu
Copy link
Contributor

dadamu commented Sep 29, 2021

@RiccardoM Okay, let me open an ADR and research more about multisig.

@RiccardoM RiccardoM changed the title desmos create-chain-link-json multisig support Support multisig address inside MsgCreateChainLink Sep 30, 2021
@RiccardoM RiccardoM added the x/profiles Module that allows to create and manage decentralized social profiles label Sep 30, 2021
@dadamu
Copy link
Contributor

dadamu commented Oct 5, 2021

@RiccardoM @bragaz Before completing the adr, I would like to discuss with what I researched and the idea how to implement the feature.

In general, there are two problem which should be solved:

1. Multisig address regeneration

it is impossible to confirm if the multisig address is right by pubkeys of signers.
For instance, there is a multisig address generated by 3 keypairs with threshold which is 2, then the create-chain-link which the user prepared is signed with only 2 pubkey. There is a problem that Desmos can't regenerate multisig address since it contains only 2 pubkey. The way of multisig generation is here.

To solve it, the chain-link-json must be signed by all keypairs involved the multisig generation.

2. The verification process of multisignature

The verification process of multisig signature is also big different from the single keypair. The multisignature contains a list of pubkeys, and a list of signatures generated by them.
The process is like:

  1. Check the number of signature and pubkeys are more than threshold
  2. Iterate signatures and verify it with pubkeys

It means we need to implement the feature to verify and store the multisignature.

Let me know what do you think, @RiccardoM @bragaz.

@RiccardoM
Copy link
Contributor

RiccardoM commented Oct 6, 2021

Thanks @dadamu for the details explanation of where the problems may lie with multisig accounts. I've taken some time to dig deeper inside the Cosmos SDK codebase, in order to understand how the multisig are handled for transaction and how we can leverage the same process for our case. Here is what I got from my journey:

  • The SDK has two different types of public keys: cryptotypes.PubKey, which represents a generic public key, and multisig.PubKey, which represents a generic multisig public key.
  • The SDK provides an interface to represent a generic signature: SignatureData. This is then implemented for both single key signatures (using SingleSignatureData) and for multi signatures (using MultiSignatureData).
  • The SDK also comes with some util methods used to convert a SignatureData instance to a Proto-compatible type, and vice-versa. These methods are: SignatureDataToProto and SignatureDataFromProto. These methods are also used when serializing/deserializing the signatures generated using the desmos tx sign and desmos tx multisig commands.
  • In order to verify a signature, the SDK uses the two public methods associated with each key type: crytptotypes.PubKey#VerifySignature and multisig.PubKey#VerifyMultisignature. You can see an example of such usage inside the VerifySignature method that is used when verifying a transaction. The first requires a SingleSignatureData instance, while the second requires a MultiSignatureData instance.

With all of these, I think I found out how we can change our code to support multisig accounts as well:

  1. Require users to provide a SignatureData instance inside the link proof.
  2. When verifying the signature, check the SignatureData type and
    1. If it's a SingleSignatureData, make sure the account public key is a cryptotypes. PubKey and then use the VerifySignature method to verify the signature.
    2. If it's a MultiSignatureData, make sure the account public key is a multisig.PubKey and then use the VerifyMultisignature method to verify the signature.

All in all, our Proof type should become something like

type Proof struct {
  PlainText []byte
  Signature SignatureData
  PublicKey PubKey
}

Notes

About SingleSignatureData

As you might note, the SingleSignatureData has a field named SignMode which contains the signature mode used when encoding the transaction during the signature. Since we require the user to provide us with the plain text that has been signed, we do not need this value and so it can be set to any value (even an invalid one).

Applicability

It should be discussed whether or not this method should be applied to chain link proofs too.

@dadamu dadamu self-assigned this Oct 7, 2021
@RiccardoM RiccardoM linked a pull request Dec 20, 2021 that will close this issue
19 tasks
@mergify mergify bot closed this as completed in #634 Dec 21, 2021
mergify bot pushed a commit that referenced this issue Dec 21, 2021
## Description



This PR introduces the draft of the 6th ADR related to Support multisig chain link in #633.

---

### 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...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://docs.cosmos.network/v0.44/building-modules/intro.html)
- [ ] included the necessary unit and integration [tests](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
@RiccardoM RiccardoM reopened this Dec 21, 2021
@mergify mergify bot closed this as completed in #708 Jan 14, 2022
mergify bot pushed a commit that referenced this issue Jan 14, 2022
## 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...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://docs.cosmos.network/v0.44/building-modules/intro.html)
- [x] included the necessary unit and integration [tests](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/new-feature Propose the addition of a new feature that does not yet exist x/profiles Module that allows to create and manage decentralized social profiles
Projects
None yet
3 participants