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

Update test CLI for new registration and signing flows #1008

Merged
merged 15 commits into from
Aug 20, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ At the moment this project **does not** adhere to
- Signing flow with derived accounts ([#990](https://github.com/entropyxyz/entropy-core/pull/990))
- TSS attestation endpoint ([#1001](https://github.com/entropyxyz/entropy-core/pull/1001))
- Add `network-jumpstart` command to `entropy-test-cli` ([#1004](https://github.com/entropyxyz/entropy-core/pull/1004))
- Update test CLI for new registration and signing flows ([#1008](https://github.com/entropyxyz/entropy-core/pull/1008))

### Changed
- Fix TSS `AccountId` keys in chainspec ([#993](https://github.com/entropyxyz/entropy-core/pull/993))
Expand Down
169 changes: 134 additions & 35 deletions crates/client/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,39 +76,40 @@ pub async fn register(
signature_request_keypair: sr25519::Pair,
program_account: SubxtAccountId32,
programs_data: BoundedVec<ProgramInstance>,
) -> Result<([u8; VERIFYING_KEY_LENGTH], RegisteredInfo), ClientError> {
// Send register transaction
put_register_request_on_chain(
api,
rpc,
signature_request_keypair.clone(),
program_account,
programs_data,
)
.await?;
on_chain: bool,
) -> Result<Vec<([u8; VERIFYING_KEY_LENGTH], RegisteredInfo)>, ClientError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

let account_registration_events = if on_chain {
put_register_request_on_chain(
api,
rpc,
signature_request_keypair.clone(),
program_account,
programs_data,
)
.await?
} else {
put_old_register_request_on_chain(
api,
rpc,
signature_request_keypair.clone(),
program_account,
programs_data,
)
.await?
};

let account_id: SubxtAccountId32 = signature_request_keypair.public().into();
let mut registration_info = vec![];
for event in account_registration_events {
let verifying_key = event.1 .0;
let registered_info = get_registered_details(api, rpc, verifying_key.clone()).await?;

for _ in 0..50 {
let block_hash = rpc.chain_get_block_hash(None).await?;
let events =
EventsClient::new(api.clone()).at(block_hash.ok_or(ClientError::BlockHash)?).await?;
let registered_event = events.find::<entropy::registry::events::AccountRegistered>();
for event in registered_event.flatten() {
// check if the event belongs to this user
if event.0 == account_id {
let registered_query = entropy::storage().registry().registered(&event.1);
let registered_status = query_chain(api, rpc, registered_query, block_hash).await?;
if let Some(status) = registered_status {
let verifying_key =
event.1 .0.try_into().map_err(|_| ClientError::BadVerifyingKeyLength)?;
return Ok((verifying_key, status));
}
}
}
std::thread::sleep(std::time::Duration::from_millis(1000));
registration_info.push((
verifying_key.try_into().map_err(|_| ClientError::BadVerifyingKeyLength)?,
registered_info,
))
}
Err(ClientError::RegistrationTimeout)

Ok(registration_info)
}

/// Request to sign a message
Expand All @@ -131,7 +132,12 @@ pub async fn sign(
auxilary_data: Option<Vec<u8>>,
) -> Result<RecoverableSignature, ClientError> {
let message_hash = Hasher::keccak(&message);
let validators_info = get_signers_from_chain(api, rpc, false).await?;

let registered_info =
get_registered_details(api, rpc, signature_verifying_key.to_vec()).await?;
let with_parent_key = registered_info.derivation_path.is_some();
let validators_info = get_signers_from_chain(api, rpc, with_parent_key).await?;

tracing::debug!("Validators info {:?}", validators_info);
let block_number = rpc.chain_get_header(None).await?.ok_or(ClientError::BlockNumber)?.number;
let signature_request = UserSignatureRequest {
Expand Down Expand Up @@ -289,19 +295,112 @@ pub async fn get_programs(
Ok(programs)
}

/// Submit a register transaction
/// Submits a transaction registering an account on-chain.
#[tracing::instrument(
skip_all,
fields(
user_account = ?signature_request_keypair.public(),
)
)]
pub async fn put_register_request_on_chain(
api: &OnlineClient<EntropyConfig>,
rpc: &LegacyRpcMethods<EntropyConfig>,
signature_request_keypair: sr25519::Pair,
deployer: SubxtAccountId32,
program_instances: BoundedVec<ProgramInstance>,
) -> Result<(), ClientError> {
let registering_tx = entropy::tx().registry().register(deployer, program_instances);
) -> Result<Vec<entropy::registry::events::AccountRegistered>, ClientError> {
tracing::debug!("Registering an account using on-chain flow.");

let registering_tx = entropy::tx().registry().register_on_chain(deployer, program_instances);
let registered_events =
submit_transaction_with_pair(api, rpc, &signature_request_keypair, &registering_tx, None)
.await?;

// Note: In the case of the new registration flow we can have many registration events for a
// single signature request account.
let registered_events: Vec<_> = registered_events
.find::<entropy::registry::events::AccountRegistered>()
.flatten()
.filter(|event| event.0 == signature_request_keypair.public().into())
.collect();

Ok(registered_events)
}

/// Submits a transaction registering an account on-chain using the old off-chain flow.
#[tracing::instrument(
skip_all,
fields(
user_account = ?signature_request_keypair.public(),
)
)]
pub async fn put_old_register_request_on_chain(
api: &OnlineClient<EntropyConfig>,
rpc: &LegacyRpcMethods<EntropyConfig>,
signature_request_keypair: sr25519::Pair,
deployer: SubxtAccountId32,
program_instances: BoundedVec<ProgramInstance>,
) -> Result<Vec<entropy::registry::events::AccountRegistered>, ClientError> {
tracing::debug!("Registering an account using old off-chain flow.");

let registering_tx = entropy::tx().registry().register(deployer, program_instances);
submit_transaction_with_pair(api, rpc, &signature_request_keypair, &registering_tx, None)
.await?;
Ok(())

let account_id: SubxtAccountId32 = signature_request_keypair.public().into();

for _ in 0..50 {
let block_hash = rpc.chain_get_block_hash(None).await?;
let events =
EventsClient::new(api.clone()).at(block_hash.ok_or(ClientError::BlockHash)?).await?;
let registered_event = events.find::<entropy::registry::events::AccountRegistered>();
for event in registered_event.flatten() {
// check if the event belongs to this user
if event.0 == account_id {
return Ok(vec![event]);
}
}
std::thread::sleep(std::time::Duration::from_millis(1000));
}

Err(ClientError::RegistrationTimeout)
}

/// Returns a registered user's key visibility
///
/// 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...
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

#[tracing::instrument(skip_all, fields(verifying_key))]
pub async fn get_registered_details(
api: &OnlineClient<EntropyConfig>,
rpc: &LegacyRpcMethods<EntropyConfig>,
verifying_key: Vec<u8>,
) -> Result<RegisteredInfo, ClientError> {
tracing::info!("Querying chain for registration info.");

let registered_info_query =
entropy::storage().registry().registered(BoundedVec(verifying_key.clone()));
let registered_result = query_chain(api, rpc, registered_info_query, None).await?;

let registration_info = if let Some(old_registration_info) = registered_result {
tracing::debug!("Found user in old `Registered` struct.");

old_registration_info
} else {
// We failed with the old registration path, let's try the new one
tracing::warn!("Didn't find user in old `Registered` struct, trying new one.");

let registered_info_query =
entropy::storage().registry().registered_on_chain(BoundedVec(verifying_key));

query_chain(api, rpc, registered_info_query, None)
.await?
.ok_or_else(|| ClientError::NotRegistered)?
};

Ok(registration_info)
}

/// Check that the verfiying key from a new signature matches that in the from the
Expand Down
19 changes: 16 additions & 3 deletions crates/test-cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ enum CliCommand {
/// If giving a mnemonic it must be enclosed in quotes, eg: "--mnemonic-option "alarm mutual concert...""
#[arg(short, long)]
mnemonic_option: Option<String>,
/// Indicates that a user wants to register using the fully on-chain registration flow.
#[arg(long)]
on_chain: bool,
},
/// Ask the network to sign a given message
Sign {
Expand All @@ -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)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ameba23 does this trigger you 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

mnemonic_option: Option<String>,
},
/// Update the program for a particular account
Expand Down Expand Up @@ -173,7 +177,7 @@ pub async fn run_command(
let rpc = get_rpc(&endpoint_addr).await?;

match cli.command {
CliCommand::Register { mnemonic_option, programs } => {
CliCommand::Register { mnemonic_option, programs, on_chain } => {
let mnemonic = if let Some(mnemonic_option) = mnemonic_option {
mnemonic_option
} else {
Expand All @@ -192,16 +196,25 @@ pub async fn run_command(
);
}

let (verifying_key, registered_info) = register(
let registrations = register(
&api,
&rpc,
program_keypair.clone(),
program_account,
BoundedVec(programs_info),
on_chain,
)
.await?;

Ok(format!("Verfiying key: {},\n{:?}", hex::encode(verifying_key), registered_info))
if registrations.is_empty() {
panic!("Failed to register an account!")
}

// The CLI currently doesn't support sending multiple registration requests in a single
Copy link
Member

Choose a reason for hiding this comment

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

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

// go, so we just grab one event (presumably the only one) and log that.
let (verifying_key, registered_info) = &registrations[0];

Ok(format!("Verifying key: {},\n{:?}", hex::encode(verifying_key), registered_info))
},
CliCommand::Sign { signature_verifying_key, message, auxilary_data, mnemonic_option } => {
let mnemonic = if let Some(mnemonic_option) = mnemonic_option {
Expand Down
Loading