feat(multisig-prover): start update worker set #67

cjcobb23 · 2023-08-25T02:23:09Z

Description

detect if worker set needs to be updated
abi encode new worker set and include in batch
depends on #64

Todos

update the worker set automatically as part of batch creation

Unit tests
Manual tests
Documentation
Connect epics/issues

Steps to Test

Expected Behaviour

Other Notes

AXE-1534

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

* detect if worker set needs to be updated * ABI encode new worker set and add to batch

cgorenflo · 2023-08-31T22:04:09Z

contracts/multisig-prover/src/encoding/evm.rs

+            .map(TryInto::try_into)
+            .collect::<Result<Vec<Command>, ContractError>>()?;
+        if new_worker_set.is_some() {
+            commands.push(new_worker_set.clone().unwrap().try_into()?);


don't use unwrap in production code

use if let Some(worker_set) instead

Pushing it into .commands without touching the .message_ids is weird to me.

cgorenflo · 2023-08-31T22:09:03Z

contracts/multisig-prover/src/encoding/evm.rs

+        .iter()
+        .map(|s| {
+            (
+                evm_address((&s.pub_key).into()).expect("couldn't convert pubkey to evm address"),


seems to me that maybe evm_address needs to change? You're throwing away the knowledge that it's a valid pub_key, only to panic if it's not valid

I partially agree with @cgorenflo. Any Ecdsa public key can be converted into a valid Ethereum address so it only makes sense for the evm_address method to return an error when the key type isn't right.

cgorenflo · 2023-08-31T22:15:37Z

contracts/multisig-prover/src/encoding/evm.rs

+    let (addresses, weights) = operators.iter().fold(
+        (vec![], vec![]),
+        |(mut addresses, mut weights), operator| {
+            addresses.push(Token::Address(ethereum_types::Address::from_slice(
+                operator.0.as_slice(),
+            )));
+            weights.push(Token::Uint(ethereum_types::U256::from_big_endian(
+                &operator.1.to_be_bytes(),
+            )));
+
+            (addresses, weights)
+        },
+    );


use unzip

cgorenflo · 2023-08-31T22:19:29Z

contracts/multisig-prover/src/encoding/evm.rs

+        .zip(snapshot.participants.values().map(|p| p.weight.into()))
+        .collect();
+    Ok(Operators {
+        weights: zipped,


call this weights_by_addresses or something similar, so it's clear that it's a tuple

cgorenflo · 2023-08-31T22:25:38Z

contracts/multisig-prover/src/types.rs

 impl BatchID {
-    pub fn new(message_ids: &[String]) -> BatchID {
+    pub fn new(message_ids: &[String], new_worker_set: Option<WorkerSet>) -> BatchID {
        let mut message_ids = message_ids.to_vec();
        message_ids.sort();

+        if let Some(new_worker_set) = new_worker_set {
+            message_ids.push(new_worker_set.hash().to_string())
+        }
        Keccak256::digest(message_ids.join(",")).as_slice().into()
    }
 }


what happens if message_ids are empty and worker_set is none?

contracts/multisig-prover/src/execute.rs

cgorenflo · 2023-09-01T00:01:04Z

contracts/multisig-prover/src/state.rs

+#[cw_serde]
+pub struct WorkerSet {
+    pub signers: Vec<Signer>,
+    pub threshold: Uint256,


which threshold is this?

signing threshold. Defines how many signatures are needed before relaying to the external gateway

cgorenflo · 2023-09-01T00:01:41Z

contracts/multisig-prover/src/state.rs

+    pub fn new(
+        snapshot: Snapshot,
+        pub_keys: Vec<multisig::types::PublicKey>,
+        env: Env,


pass in the block height instead

Agree with @cgorenflo. This goes again with my comment saying that I don't like passing exessive information to any function.

cgorenflo · 2023-09-01T00:03:58Z

contracts/multisig/src/contract.rs

            })
-            .collect::<Vec<Signer>>();
+            .collect::<Vec<(Signer, Option<Signature>)>>();


let signers_with_sigs = ...

cgorenflo · 2023-09-01T00:04:36Z

contracts/multisig/src/msg.rs

 }

 #[cw_serde]
 pub struct Multisig {
    pub state: MultisigState,
    pub quorum: Uint256,
-    pub signers: Vec<Signer>,
+    pub signers: Vec<(Signer, Option<Signature>)>,


Suggested change

pub signers: Vec<(Signer, Option<Signature>)>,

pub signers_with_sigs: Vec<(Signer, Option<Signature>)>,

why are signatures optional?

This is the response for the GetMultisig query, and the behavior is to return all of the signers, even if they haven't signed a particular message yet. So the signature is present iff they signed.

fish-sammy · 2023-09-01T12:36:57Z

contracts/multisig-prover/src/encoding/evm.rs

@@ -44,34 +46,69 @@ impl TryFrom<Message> for Command {
    }
 }

-impl TryFrom<Signer> for Operator {
+impl TryFrom<WorkerSet> for Command {


I don't think implementing From or TryFrom simply cuz something can be converted to another makes sense to me. Personally, it still has to be two things that are conceptually the same or similar. I'd rather seeing a new_transfer_operatorship method that takes a WorkerSet.

contracts/multisig-prover/src/state.rs

fish-sammy · 2023-09-01T12:48:35Z

contracts/multisig-prover/src/encoding/evm.rs

+    }
+}
+
+impl TryFrom<(Signer, Option<Signature>)> for Operator {


So this is another example where I do NOT think TryFrom makes sense. I'd prefer having impl TryFrom<Signer> for Operator and then have a method like .signature( ... ) on Operator to set it.

fish-sammy · 2023-09-01T12:51:07Z

contracts/multisig-prover/src/encoding/evm.rs

        })
    }
 }

+pub fn make_worker_set(


This function isn't being used?

Also, why is this not a method on WorkerSet? I also don't like it taking an entire Snapshot as one of the parameters cuz only one field from the snapshot is relevant here.

It's not being used, deleted it. There is a similar method on WorkerSet actually

fish-sammy · 2023-09-01T13:01:15Z

contracts/multisig-prover/src/encoding/evm.rs

 impl CommandBatch {
    pub fn new(
        messages: Vec<Message>,
        destination_chain_id: Uint256,
+        new_worker_set: Option<WorkerSet>,


I'd prefer something like following instead of having everything possible in the .new method.

let command_batch_builder = CommandBatchBuilder::default(); command_batch_builder.add_message( ... ); command_batch_builder.add_new_worker_set( ... ); let command_batch = command_batch_builder.build();

We are likely going to have more types of commands, and keep adding things to new is confusing to me.

fish-sammy · 2023-09-01T13:09:48Z

contracts/multisig-prover/src/contract.rs

@@ -41,6 +41,7 @@ pub fn instantiate(
        service_name: msg.service_name,
        chain_name: ChainName::from_str(&msg.chain_name)
            .map_err(|_| ContractError::InvalidChainName)?,
+        worker_set_diff_threshold: msg.worker_set_diff_threshold,


What is this?

It's the max number of workers that can change without triggering a worker set update.

fish-sammy · 2023-09-01T13:14:30Z

contracts/multisig-prover/src/execute.rs

+    let mut pub_keys = vec![];
+    for worker in &workers {
+        let pub_key_query = multisig::msg::QueryMsg::GetPublicKey {
+            worker_address: worker.address.to_string(),
+            key_type: multisig::types::KeyType::ECDSA,
+        };
+        let pub_key: PublicKey = deps.querier.query(&QueryRequest::Wasm(WasmQuery::Smart {
+            contract_addr: config.multisig.to_string(),
+            msg: to_binary(&pub_key_query)?,
+        }))?;
+        pub_keys.push(pub_key);
+    }


Do this with .iter().map?

contracts/multisig-prover/src/execute.rs

fish-sammy · 2023-09-01T13:18:19Z

contracts/multisig-prover/src/state.rs

+    pub fn new(
+        snapshot: Snapshot,
+        pub_keys: Vec<multisig::types::PublicKey>,
+        env: Env,


Agree with @cgorenflo. This goes again with my comment saying that I don't like passing exessive information to any function.

fish-sammy · 2023-09-01T13:19:10Z

contracts/multisig-prover/src/state.rs

+    }
+
+    pub fn hash(&self) -> HexBinary {
+        Keccak256::digest(serde_json::to_vec(&self).expect("couldn't serialize worker set"))


Here it makes it clearer that a BTreeSet makes way more sense than a Vec cuz order matters when it comes to the hash.

…-set

# Conflicts: # ampd/src/handlers/multisig.rs # contracts/multisig-prover/src/execute.rs # contracts/multisig-prover/src/test/test_data.rs # contracts/multisig/src/contract.rs # contracts/multisig/src/key.rs

cjcobb23 and others added 2 commits August 22, 2023 16:39

feat(multisig): public key registration

990e0c0

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

Merge branch 'main' into feat/multisig-key-registration

8c3d082

cjcobb23 requested a review from a team as a code owner August 25, 2023 02:23

cjcobb23 mentioned this pull request Aug 26, 2023

feat(prover): confirm worker set #70

Merged

cjcobb23 added 3 commits August 31, 2023 09:59

Merge branch 'main' into feat/multisig-key-registration

30e0838

fix tests

9b9344e

refactor

1a47603

cjcobb23 changed the base branch from main to feat/multisig-key-registration August 31, 2023 17:50

feat(multisig-prover): start update worker set

ce76b67

* detect if worker set needs to be updated * ABI encode new worker set and add to batch

cjcobb23 force-pushed the feat/update-worker-set branch from 58781f4 to ce76b67 Compare August 31, 2023 19:06

cgorenflo requested changes Sep 1, 2023

View reviewed changes

fish-sammy requested changes Sep 1, 2023

View reviewed changes

cjcobb23 and others added 13 commits September 1, 2023 10:46

address review

0a153f0

review

b46cd9a

address review

902bf3f

address review

e34f24c

Merge branch 'feat/multisig-key-registration' into feat/update-worker…

15e3f38

…-set

address review

789ea50

address review

7faa587

add missing file

5d1ff1d

remove unnecessary trait

ba32ec5

Merge branch 'feat/multisig-key-registration' into feat/update-worker…

d893cac

…-set

Merge branch 'main' into feat/multisig-key-registration

e1a35d3

Merge branch 'feat/multisig-key-registration' into feat/update-worker…

8ac1908

…-set

rename struct field according to contract renaming

b67c7f9

eguajardo added the ceremony label Sep 7, 2023

cgorenflo approved these changes Sep 7, 2023

View reviewed changes

Base automatically changed from feat/multisig-key-registration to main September 7, 2023 23:35

cgorenflo added 2 commits September 7, 2023 19:41

Merge branch 'main' into feat/update-worker-set

197b0e1

# Conflicts: # ampd/src/handlers/multisig.rs # contracts/multisig-prover/src/execute.rs # contracts/multisig-prover/src/test/test_data.rs # contracts/multisig/src/contract.rs # contracts/multisig/src/key.rs

merge conflicts fixed

909f9ae

cgorenflo merged commit 1eace6b into main Sep 8, 2023
3 checks passed

cgorenflo deleted the feat/update-worker-set branch September 8, 2023 00:10

cgorenflo restored the feat/update-worker-set branch September 8, 2023 00:55

cgorenflo deleted the feat/update-worker-set branch September 12, 2023 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(multisig-prover): start update worker set #67

feat(multisig-prover): start update worker set #67

cjcobb23 commented Aug 25, 2023 •

edited

Loading

cgorenflo Aug 31, 2023

cgorenflo Aug 31, 2023

fish-sammy Sep 1, 2023

cgorenflo Aug 31, 2023

fish-sammy Sep 1, 2023

cgorenflo Aug 31, 2023

cgorenflo Aug 31, 2023

cgorenflo Aug 31, 2023

cgorenflo Sep 1, 2023

cjcobb23 Sep 1, 2023

cgorenflo Sep 1, 2023

fish-sammy Sep 1, 2023

cgorenflo Sep 1, 2023

cgorenflo Sep 1, 2023

cgorenflo Sep 1, 2023

cjcobb23 Sep 1, 2023

fish-sammy Sep 1, 2023

fish-sammy Sep 1, 2023

fish-sammy Sep 1, 2023

fish-sammy Sep 1, 2023

cjcobb23 Sep 1, 2023

fish-sammy Sep 1, 2023

fish-sammy Sep 1, 2023

fish-sammy Sep 1, 2023

cjcobb23 Sep 1, 2023

fish-sammy Sep 1, 2023

fish-sammy Sep 1, 2023

fish-sammy Sep 1, 2023 •

edited

Loading

	pub signers: Vec<(Signer, Option<Signature>)>,
	pub signers_with_sigs: Vec<(Signer, Option<Signature>)>,

feat(multisig-prover): start update worker set #67

feat(multisig-prover): start update worker set #67

Conversation

cjcobb23 commented Aug 25, 2023 • edited Loading

Description

Todos

Steps to Test

Expected Behaviour

Other Notes

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fish-sammy Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

cjcobb23 commented Aug 25, 2023 •

edited

Loading

fish-sammy Sep 1, 2023 •

edited

Loading