Update test CLI for new registration and signing flows #1008

HCastano · 2024-08-15T13:51:23Z

This PR adds support for the new registration and signing flows to the
entropy-test-cli. To do so I added a new flag to the register command, --on-chain
which indicates that a user wants to register using the new flow. If the flag is not
added the old flow will continue to be used.

For signing, a user doesn't have to do anything special. They can use the same command as
before and the CLI will resolve which flow to use internally.

To try this out on a local network:

Run docker compose up, I've been using ones built from the branch locally, but the
images from master should work too.
cargo run -p entropy-test-cli -- jumpstart-network, this needs to be done before the
new registration flow is available
cargo run -p entropy-test-cli -- register ./crates/testing-utils/template_barebones.wasm -m //Alice --on-chain
cargo run -p entropy-test-cli -- sign $VERIFYING_KEY "Hello, GitHub!"

HCastano · 2024-08-15T13:52:05Z

crates/client/src/client.rs

+/// TODO (Nando): This was copied from `entropy-tss::helpers::substrate`
+///
+/// What's the best place for this? Ideally here, and then we import this into the entropy-tss, but
+/// we need the `full-client` feature enabled...


@ameba23 you got any thoughts on the best place to put this is? I'd rather not have this duplicated throughout

The idea is that this client file is for things which are only used client-side, and since entropy-tss doesn't need them apart from in testing, they are behind the full-client feature.

Anything outside of this file is accessible to both client and server, so this could go in user.rs (or substrate.rs since it is currently in helpers::substrate in entropy-tss)

HCastano · 2024-08-15T13:52:22Z

crates/test-cli/src/lib.rs

@@ -89,6 +92,7 @@ enum CliCommand {
        /// Optional auxiliary data passed to the program, given as hex
        auxilary_data: Option<String>,
        /// The mnemonic to use for the call
+        #[arg(short, long)]


@ameba23 does this trigger you 😆

yes. well less so for sign because it doesn't really matter what this is. but for the commands which submit extrinsics, it seems insane that its 'optional' to specify which account to use.

Since we do use the CLI for development purposes it's not that crazy- we just default to //Alice otherwise. But yeah should definitely try and clarify the UX around this at some point

JesseAbram · 2024-08-15T21:33:37Z

crates/test-cli/src/lib.rs

+                panic!("Failed to register an account!")
+            }
+
+            // The CLI currently doesn't support sending multiple registration requests in a single


nor really should it, may be cleaner to just return the first registration from the above function

ameba23

Great to see this all working ⭐

Can confirm the docker-compose stuff works my end.

ameba23 · 2024-08-16T06:01:56Z

crates/client/src/client.rs

+/// TODO (Nando): This was copied from `entropy-tss::helpers::substrate`
+///
+/// What's the best place for this? Ideally here, and then we import this into the entropy-tss, but
+/// we need the `full-client` feature enabled...


The idea is that this client file is for things which are only used client-side, and since entropy-tss doesn't need them apart from in testing, they are behind the full-client feature.

Anything outside of this file is accessible to both client and server, so this could go in user.rs (or substrate.rs since it is currently in helpers::substrate in entropy-tss)

ameba23 · 2024-08-16T06:29:58Z

crates/client/src/client.rs

-    )
-    .await?;
+    on_chain: bool,
+) -> Result<Vec<([u8; VERIFYING_KEY_LENGTH], RegisteredInfo)>, ClientError> {


I don't think this should return a vec. When i call register, i am interested in getting the verifying key of the account which has the configuration (programs and program configurations) that i give as arguments. I don't want to get it together with some other accounts i registered before and need to sift through them and find the one i am interested in. That should happen in this function.

If i call this function twice with with two identical configurations in the same block, there should be some mechanism to ensure that this function will not return the same verifying key for each. Eg: it should return the account with the lowest / highest derivation path.

I don't think this should return a vec. When i call register, i am interested in getting the verifying key of the account which has the configuration (programs and program configurations) that i give as arguments. I don't want to get it together with some other accounts i registered before and need to sift through them and find the one i am interested in. That should happen in this function.
I figured that in the future we could allow the possibility for a register-batch command where a single account could register a lot of accounts on-chain in one go. This is supported by the on-chain flow.

The register command as it stands now only sends one transaction per invocation anyways, so this isn't really a problem.

If i call this function twice with with two identical configurations in the same block, there should be some mechanism to ensure that this function will not return the same verifying key for each. Eg: it should return the account with the lowest / highest derivation path.

While this is easy right now since we have a count on-chain, I'd hesitate to make that assumption in the TSS since we could change the way we do derivations. Not sure if this filtering is something I'll implement for this PR, but I'll think about it as I go through the fixes

I quickly tried to send a few transactions back to back but it doesn't work atm because of how the nonce management is done for the registering account. So I guess technically this isn't an issue then lol

ok well i don't know if thats good or bad - but im happy that this now returns just one set of registering details.

Does that mean we can make at most one registration per account per block? Doesn't seem so bad but i might have misunderstood the issue.

@ameba23 no, the chain can support multiple registrations per account per block. It's just the test CLI that doesn't

ameba23 · 2024-08-16T06:34:23Z

crates/test-cli/src/lib.rs

@@ -89,6 +92,7 @@ enum CliCommand {
        /// Optional auxiliary data passed to the program, given as hex
        auxilary_data: Option<String>,
        /// The mnemonic to use for the call
+        #[arg(short, long)]


yes. well less so for sign because it doesn't really matter what this is. but for the commands which submit extrinsics, it seems insane that its 'optional' to specify which account to use.

HCastano added 9 commits August 14, 2024 18:03

First pass at new registration flow

d0cbcd3

Hack jumpstart during registration flow

b5d7b6e

Split regstration into two functions depending on old or new flow

248fa6a

Only do jumpstart if using the on-chain flow

eb39a1f

Support signing for both old and new flows

ab59939

Make mnemonic_option consistent across registration and signing

af47e50

Clean up a few things in the register flow

fd4a600

Panic if we didn't get any successful registrations

139c893

Remove hacked jumpstart from registration flow

61610c2

HCastano commented Aug 15, 2024

View reviewed changes

Move comment around

326ee16

HCastano requested review from ameba23 and JesseAbram August 15, 2024 13:53

Add CHANGELOG entry

b6245ea

JesseAbram approved these changes Aug 15, 2024

View reviewed changes

ameba23 approved these changes Aug 16, 2024

View reviewed changes

HCastano added 4 commits August 20, 2024 14:21

Merge branch 'master' into hc/update-test-cli

57a7c15

Only return single registration event

75833d6

Move get_registered_details fully into client

837467c

Update error used in signing test

032e07d

HCastano merged commit bba31a3 into master Aug 20, 2024
8 checks passed

HCastano deleted the hc/update-test-cli branch August 20, 2024 21:01

This was referenced Aug 20, 2024

Add test showing register and sign with test client #1012

Merged

Registration and signing flow follow-ups #1033

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update test CLI for new registration and signing flows #1008

Update test CLI for new registration and signing flows #1008

HCastano commented Aug 15, 2024

HCastano Aug 15, 2024

ameba23 Aug 16, 2024

HCastano Aug 15, 2024

ameba23 Aug 16, 2024

HCastano Aug 20, 2024

JesseAbram Aug 15, 2024

ameba23 left a comment •

edited

Loading

ameba23 Aug 16, 2024

ameba23 Aug 16, 2024

HCastano Aug 20, 2024

HCastano Aug 20, 2024

ameba23 Aug 21, 2024

HCastano Aug 21, 2024

ameba23 Aug 16, 2024

Update test CLI for new registration and signing flows #1008

Update test CLI for new registration and signing flows #1008

Conversation

HCastano commented Aug 15, 2024

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

ameba23 left a comment • edited Loading

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

ameba23 left a comment •

edited

Loading