-
Notifications
You must be signed in to change notification settings - Fork 1
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
Changes from 12 commits
d0cbcd3
b5d7b6e
248fa6a
eb39a1f
ab59939
af47e50
fd4a600
139c893
61610c2
326ee16
b6245ea
57a7c15
75833d6
837467c
032e07d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> { | ||
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 | ||
|
@@ -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 { | ||
|
@@ -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, ®istering_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, ®istering_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... | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea is that this Anything outside of this file is accessible to both client and server, so this could go in |
||
#[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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
|
@@ -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)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ameba23 does this trigger you 😆 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes. well less so for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
mnemonic_option: Option<String>, | ||
}, | ||
/// Update the program for a particular account | ||
|
@@ -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 { | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) = ®istrations[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 { | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
register
command as it stands now only sends one transaction per invocation anyways, so this isn't really a problem.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 fixesThere was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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