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(multisig): public key registration #64

Merged
merged 15 commits into from
Sep 7, 2023

Conversation

cjcobb23
Copy link
Contributor

@cjcobb23 cjcobb23 commented Aug 22, 2023

Description

  • Add a method to register public keys
  • Add a query to fetch public keys
  • Distinguish keys and signatures by type

Todos

  • Unit tests
  • Manual tests
  • Documentation
  • Connect epics/issues

Steps to Test

Expected Behaviour

Other Notes

AXE-1533

* Add a method to register public keys
* Add a query to fetch public keys
* Distinguish keys and signatures by type
@cjcobb23 cjcobb23 requested a review from a team as a code owner August 22, 2023 20:41
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure about those locations? Why do two line inserts shift the lines by 3?

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 was wrong, it was supposed to be 2.

@@ -51,7 +51,7 @@ impl TryFrom<Signer> for Operator {
Ok(Self {
address: evm_address((&signer.pub_key).into())?,
weight: signer.weight,
signature: signer.signature,
signature: signer.signature.map(|Signature::ECDSA(sig)| sig),
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like I didn't see this when it was added. Why does the signer struct have an optional signature?

Copy link
Contributor

Choose a reason for hiding this comment

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

same goes for the operator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was from before, but it's due to the way Signer is used. It's returned by the GetMultisig query, which queries the status of a specific signing session. So it returns the signer, and the signature (if present).

I actually refactored this in a later PR to move the signature out of the Signer struct.

let signature: Signature = match key
.pub_keys
.iter()
.find(|pk| pk.0 == &info.sender.to_string())
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way to do this comparison cleaner?

Copy link
Contributor

Choose a reason for hiding this comment

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

was changed by CJ to .find(|&(addr, _)| addr == &info.sender.to_string()). Looks good to me

signer: info.sender.into(),
})
}
Some((_, pk)) => (pk.clone(), signature).try_into()?,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we throw away the key type here?

Comment on lines 15 to 17
pub enum KeyType {
ECDSA,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why do KeyType and PublicKey need to be separate entities?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need KeyType on it's own at least for the query, and possibly for the execute msg

contracts/multisig/src/types.rs Outdated Show resolved Hide resolved
pub_keys: pub_keys
.clone()
.into_iter()
.map(|(k, v)| (k, (KeyType::ECDSA, v)))
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the type information not part of the arguments of rotate_snapshot. I 100% guarantee you, this hardcoding is going to break the contract if we ever add another key type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rotate snapshot is actually deleted in a later PR

contracts/multisig/src/msg.rs Outdated Show resolved Hide resolved
contracts/multisig/src/state.rs Outdated Show resolved Hide resolved
contracts/multisig/src/state.rs Outdated Show resolved Hide resolved
pub_keys: pub_keys
.clone()
.into_iter()
.map(|(k, v)| (k, (KeyType::ECDSA, v)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just wanna confirm, later on the key type is gonna be passed in for rotate_snapshot? Is that the plan?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We won't need rotate_snapshot later on, it's actually deleted in a later PR. We can just pass PublicKey in KeyGen execute message, if we want to keep KeyGen

contracts/multisig/src/types.rs Outdated Show resolved Hide resolved
contracts/multisig/src/contract.rs Outdated Show resolved Hide resolved

pub trait VerifiableSignature {
fn verify(&self, msg: &MsgToSign, pub_key: &PublicKey) -> Result<bool, ContractError>;
}

#[cw_serde]
pub struct PublicKey(HexBinary);
pub enum KeyType {
Copy link
Collaborator

@fish-sammy fish-sammy Aug 31, 2023

Choose a reason for hiding this comment

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

Discussed with Chris as well, here's what we came up with to make these nicer. Warning: pseudocode.

enum KeyType {
  ECDSA,
  ED25519,
}

enum Signature {
  ECDSA(HexBinary)
  ED25519(HexBinary)
}

enum PublicKey {
    ECDSA(HexBinary)
    ED25519(HexBinary)
}

trait KeyTyped {
  fn matches<T>(&self, other: T) -> bool where T: KeyTyped {
    self.key_type() == other.key_type()
  };

  fn key_type(&self) -> KeyType;
}

impl KeyTyped for PublicKey {
  fn key_type(&self) -> KeyType {
    match self {
      ECDSA(_) => KeyType::ECDSA,
      ED25519(_) => KeyType::ED25519,
    }
  }
}

impl KeyTyped for Signature {
  fn key_type(&self) -> KeyType {
    match self {
      ECDSA(_) => KeyType::ECDSA,
      ED25519(_) => KeyType::ED25519,
    }
  }
}

impl Signature {
  pub fn verify(&self, pub_key: PublicKey) -> Result<bool, Error> {
    if !self.matches(pub_key) {
      return Err(Error::new("Key type mismatch"));
    }

    match (self, pub_key) {
      (ECDSA(sig), ECDSA(pub_key)) => {
        todo!()
      },
      (ED25519(sig), ED25519(pub_key)) => {
        todo!()
      },
      _ => {
        panic!("Key type mismatch");
      }
    }
  }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@cjcobb23 Also, you need to check out this https://serde.rs/enum-representations.html. I personally prefer it being internally tagged, i.e. #[serde(tag = "type")].

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 implemented this, take a look

contracts/multisig/src/types.rs Outdated Show resolved Hide resolved
contracts/multisig/src/msg.rs Outdated Show resolved Hide resolved
contracts/multisig/src/events.rs Outdated Show resolved Hide resolved
@cgorenflo cgorenflo merged commit 148dd9c into main Sep 7, 2023
3 checks passed
@cgorenflo cgorenflo deleted the feat/multisig-key-registration branch September 7, 2023 23:35
@cgorenflo cgorenflo restored the feat/multisig-key-registration branch September 8, 2023 00:54
@cgorenflo cgorenflo deleted the feat/multisig-key-registration branch September 12, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants