Moved multisig to its own crate #3147

j4m1ef0rd · 2023-04-27T07:03:48Z

Pull Request

Closes: PRO-101
Closes: #3022

Checklist

I am confident that the code works.
I have updated documentation where appropriate.

Summary

Moved multisig to a new crate.
Moved a few things into multisig:
- moved pubkey_to_eth_addr to multisig crate
- moved logging tags to failure reason
- moved OutgoingMultisigStageMessages && ProtocolVersion to multisig::p2p
Moved a few things out of multisig
- moved CEREMONY_ID_WINDOW to client
- moved test logger to utilities crate so it can be used in the multisig crate
Changed a few things to be in feature flag "test" instead of cfg test so they could be used in the engine crate
- movedget_key_data_for_test from helpers to keygen_detail. (because helpers is cfg test)

Result

The build time of the engine seems to be unaffected, around 10sec before and after this PR.
Making changes to the multisig files will cause an incremental test build time of around 4sec instead of 10sec before this PR.

Note: build times in the above image are taken using cargo build --timing with the selected package. So the times are different from my quoted test build times that are from cargo test ...

- moved some things around - added feature flag 'test' to multisig crete

codecov · 2023-04-27T07:16:36Z

Codecov Report

Merging #3147 (7bf7e05) into main (6e972c1) will increase coverage by 0.00%.
The diff coverage is 45.32%.

@@           Coverage Diff           @@
##             main    #3147   +/-   ##
=======================================
  Coverage   68.52%   68.53%           
=======================================
  Files         314      314           
  Lines       48807    48816    +9     
  Branches    48807    48816    +9     
=======================================
+ Hits        33445    33454    +9     
  Misses      10602    10602           
  Partials     4760     4760

Impacted Files	Coverage Δ
...gine/multisig/src/client/ceremony_manager/tests.rs	`97.46% <ø> (ø)`
engine/multisig/src/client/ceremony_runner.rs	`82.10% <ø> (ø)`
...ngine/multisig/src/client/ceremony_runner/tests.rs	`90.71% <ø> (ø)`
engine/multisig/src/client/common/broadcast.rs	`85.82% <ø> (ø)`
...ltisig/src/client/common/broadcast_verification.rs	`99.14% <ø> (ø)`
...ngine/multisig/src/client/common/ceremony_stage.rs	`100.00% <ø> (ø)`
...ngine/multisig/src/client/common/failure_reason.rs	`28.57% <ø> (ø)`
engine/multisig/src/client/key_store_api.rs	`50.00% <ø> (ø)`
engine/multisig/src/client/keygen/keygen_data.rs	`76.54% <ø> (ø)`
...ne/multisig/src/client/keygen/keygen_data/tests.rs	`100.00% <ø> (ø)`
... and 49 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

linear · 2023-04-27T07:22:30Z

PRO-101 Investigate putting multisig in its own create

Description

See if compile time when only making changes to the multisig stuff is faster.
THis would be nice because we often make small changes to unit tests and then run them.

kylezs · 2023-04-27T15:09:42Z

Making changes to the multisig files will cause an incremental build time of around 4sec instead of 10sec before this PR.

Would be nice to describe how exactly these times were measured for posterity's sake.

engine/src/constants.rs

engine/src/eth/utils.rs

multisig/src/lib.rs

msgmaxim · 2023-04-28T01:53:51Z

multisig/src/lib.rs

+	}
+
+	#[derive(Debug)]
+	pub struct VersionedCeremonyMessage {


This one probably doesn't belong in multisig, but I see that it is needed for tests, so I'm fine this leaving it here for now.

multisig/src/client/keygen/keygen_detail.rs

multisig/src/client/ceremony_manager.rs

engine/src/multisig.rs

Cargo.toml

multisig/Cargo.toml

engine/generate-genesis-keys/Cargo.toml

AlastairHolmes

The only thing I'm really concerned with it the cfg feature = test thing. What do you mean it will may cascade? Merge now without any further changes if you like. But all the comments should be responded to.

AlastairHolmes · 2023-05-01T06:19:50Z

utilities/src/testing.rs

+	}
+}
+
+use std::{


Why not put the imports at the top of the file?

AlastairHolmes · 2023-05-01T06:24:40Z

engine/multisig/src/client/keygen.rs

@@ -5,9 +5,10 @@ mod keygen_stages;
 #[cfg(test)]
 mod tests;

-#[cfg(test)]
+#[cfg(feature = "test")]


Why use this instead of cfg(test)?

We can't share cfg test stuff to other crates.
rust-lang/cargo#8379

AlastairHolmes · 2023-05-01T06:32:32Z

engine/src/logging.rs

 pub const ETH_STREAM_BEHIND: &str = "eth-stream-behind";

-use tracing::{metadata::LevelFilter, Level};
-use tracing_subscriber::{EnvFilter, Layer};


I would suggest moving the entire logger (Whenever you like), I'm guessing the api and other tools will need it.

Not much left in the file, so i moved them out and deleted the file.

fix: multisig use error from merge. chore: removed unneeded pub use of init_test_logger

- removed logger.rs file

* origin/main: feat: witness XCallNative and XCallToken (#3171) Additional Key Handover Tests (#3165) refactor/chore: remove db migration from 0 to 1 (#3166) fix: use correct stage name for PubkeyShares0 PRO-259 (#3167) ci: add timeout to post-checks ⏳ (#3169) chore: parallel post-check and publish (#3168) fix: add missing features on dep in utils (#3164) Moved multisig to its own crate (#3147) Refactor/database (#3150) fix: correct addresses for localnets (#3162) refactor/scc (#3078) fix: remove cancel workflow (#3160) refactor: remove epoch authority count (#3154) chore: remove mentions of sui (#3158) fix: remove duplicate role check (#3157) chore: remove unused quickcheck dep (#3156) feat: refactor Chainflip trait (#3144) chore: remove unused CFE deps (#3152) # Conflicts: # state-chain/pallets/cf-swapping/src/mock.rs

j4m1ef0rd added 4 commits April 27, 2023 16:54

chore: moved multisig files to new create

325b8a6

feat: fixed all uses and deps

1f17ce3

- moved some things around - added feature flag 'test' to multisig crete

chore: cleanup cargo.toml files

aebdde3

chore: moved test logger to utilities crate

7219300

j4m1ef0rd added the CFE label Apr 27, 2023

j4m1ef0rd requested review from msgmaxim and AlastairHolmes April 27, 2023 07:03

j4m1ef0rd self-assigned this Apr 27, 2023

msgmaxim reviewed Apr 28, 2023

View reviewed changes

multisig/Cargo.toml Outdated Show resolved Hide resolved

j4m1ef0rd added 6 commits April 28, 2023 16:18

chore: small changes from PR comments

d9caa9c

chore: moved multisig crate into engine folder

8a2fe85

chore: change use of multisig in multisig crate

d108c57

chore: removed re-export of multisig in the engine

0f5a068

Merge branch 'main' into feat/multisig-crate2

c01255b

Merge branch 'main' into feat/multisig-crate2

79ed992

msgmaxim approved these changes May 1, 2023

View reviewed changes

engine/generate-genesis-keys/Cargo.toml Outdated Show resolved Hide resolved

j4m1ef0rd added 2 commits May 1, 2023 13:15

chore: group local deps in toml

a85e5eb

Merge branch 'main' into feat/multisig-crate2

133e2ff

AlastairHolmes approved these changes May 1, 2023

View reviewed changes

j4m1ef0rd added 2 commits May 2, 2023 10:16

chore: group use's in test utils.

acaa815

fix: multisig use error from merge. chore: removed unneeded pub use of init_test_logger

chore: moved json logger to utilities crate

7bf7e05

- removed logger.rs file

j4m1ef0rd merged commit cf746d0 into main May 2, 2023

j4m1ef0rd deleted the feat/multisig-crate2 branch May 2, 2023 02:16

j4m1ef0rd mentioned this pull request May 4, 2023

fix: enable trace logs from multisig crate #3177

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved multisig to its own crate #3147

Moved multisig to its own crate #3147

j4m1ef0rd commented Apr 27, 2023 •

edited

Loading

codecov bot commented Apr 27, 2023 •

edited

Loading

linear bot commented Apr 27, 2023

Description

kylezs commented Apr 27, 2023

msgmaxim Apr 28, 2023

AlastairHolmes left a comment

AlastairHolmes May 1, 2023

AlastairHolmes May 1, 2023

AlastairHolmes May 1, 2023

j4m1ef0rd May 1, 2023

AlastairHolmes May 1, 2023

j4m1ef0rd May 2, 2023

Moved multisig to its own crate #3147

Moved multisig to its own crate #3147

Conversation

j4m1ef0rd commented Apr 27, 2023 • edited Loading

Pull Request

Checklist

Summary

Result

codecov bot commented Apr 27, 2023 • edited Loading

Codecov Report

linear bot commented Apr 27, 2023

Description

kylezs commented Apr 27, 2023

msgmaxim Apr 28, 2023

Choose a reason for hiding this comment

AlastairHolmes left a comment

Choose a reason for hiding this comment

AlastairHolmes May 1, 2023

Choose a reason for hiding this comment

AlastairHolmes May 1, 2023

Choose a reason for hiding this comment

AlastairHolmes May 1, 2023

Choose a reason for hiding this comment

j4m1ef0rd May 1, 2023

Choose a reason for hiding this comment

AlastairHolmes May 1, 2023

Choose a reason for hiding this comment

j4m1ef0rd May 2, 2023

Choose a reason for hiding this comment

j4m1ef0rd commented Apr 27, 2023 •

edited

Loading

codecov bot commented Apr 27, 2023 •

edited

Loading