-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat: Adding import from seed for wallet recovery web client #710
base: next
Are you sure you want to change the base?
Changes from all commits
e776b7c
3673e7f
6e34096
4d280d3
b962c76
af13505
77caf0d
81494e8
fb9c23d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,7 @@ use miden_objects::{account::AuthSecretKey, crypto::rand::FeltRng, Word}; | |
|
||
use super::Client; | ||
use crate::{ | ||
rpc::domain::account::AccountDetails, | ||
store::{AccountRecord, AccountStatus}, | ||
ClientError, | ||
}; | ||
|
@@ -251,6 +252,20 @@ impl<R: FeltRng> Client<R> { | |
.await? | ||
.ok_or(ClientError::AccountDataNotFound(account_id)) | ||
} | ||
|
||
/// Attempts to retrieve an [AccountDetails] by the [AccountId] associated with the account from | ||
/// the rpc. | ||
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. nit: rpc -> RPC (also applies to the line under the errors section) |
||
/// | ||
/// # Errors | ||
/// | ||
/// - If the key is not found on the rpc from passed `account_id`. | ||
/// - If the underlying rpc operation fails | ||
pub async fn get_account_details( | ||
&mut self, | ||
account_id: AccountId, | ||
) -> Result<AccountDetails, ClientError> { | ||
self.rpc_api.get_account_update(account_id).await.map_err(ClientError::RpcError) | ||
} | ||
Comment on lines
+263
to
+268
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. If we add something like |
||
} | ||
|
||
// TESTS | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,13 @@ impl AccountDetails { | |
Self::Private(_, summary) | Self::Public(_, summary) => summary.hash, | ||
} | ||
} | ||
|
||
pub fn account(&self) -> Option<&Account> { | ||
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. This method should be documented 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. Agreed, let's document this and the above |
||
match self { | ||
Self::Private(..) => None, | ||
Self::Public(account, _) => Some(account), | ||
} | ||
} | ||
} | ||
|
||
// ACCOUNT UPDATE SUMMARY | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,62 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use miden_client::{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
account::{Account, AccountBuilder, AccountType}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
crypto::{RpoRandomCoin, SecretKey}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Client, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use miden_lib::account::{auth::RpoFalcon512, wallets::BasicWallet}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use miden_objects::Felt; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use rand::{rngs::StdRng, Rng, SeedableRng}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use wasm_bindgen::JsValue; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
use crate::models::account_storage_mode::AccountStorageMode; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
pub async fn generate_account( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
client: &mut Client<RpoRandomCoin>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
storage_mode: &AccountStorageMode, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
mutable: bool, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
seed: Option<Vec<u8>>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) -> Result<(Account, [Felt; 4], SecretKey), JsValue> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+13
to
+18
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. It seems like this function is specifically for generating wallet accounts. If so, I would probably name this Also, it may make sense to change the signature to something like: pub async fn build_account(
seed: Vec<u8>,
storage_mode: AccountStorageMode,
mutable: bool,
anchor: &BlockHeader,
) -> Result<(Account, [Felt; 4], SecretKey), JsValue> This would simplify the internal logic by making the caller responsible for generating the seed and providing the anchor block (which could be just the genesis block for now). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let mut rng = match seed { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Some(seed_bytes) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if seed_bytes.len() == 32 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let mut seed_array = [0u8; 32]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
seed_array.copy_from_slice(&seed_bytes); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let mut std_rng = StdRng::from_seed(seed_array); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let coin_seed: [u64; 4] = std_rng.gen(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
&mut RpoRandomCoin::new(coin_seed.map(Felt::new)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Err(JsValue::from_str("Seed must be exactly 32 bytes"))? | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
None => client.rng(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+19
to
+32
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. nit: I think we can leverage
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let key_pair = SecretKey::with_rng(&mut rng); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let mut init_seed = [0u8; 32]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rng.fill_bytes(&mut init_seed); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let account_type = if mutable { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
AccountType::RegularAccountUpdatableCode | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
AccountType::RegularAccountImmutableCode | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+38
to
+42
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. Should this be generalized for any 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. That makes sense. If you don't mind I can add that as an issue for a fast follow, since we'll have to create custom wasm structs for the AccountType enums (they dont exist atm). Just to keep this commit from getting too big |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let anchor_block = client.get_latest_epoch_block().await.unwrap(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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. We should add a 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. Another option could be to always build the accounts using genesis block as the anchor. This is probably fine for now and would give deterministic results. 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. This makes sense. Looking at examples for getting the genesis block header, I see that the existing way is to query the rpc api:
The web client doesnt have access atm to invoke api calls at will (it has to use an exposed rust client call). So what I'm wondering is if it makes sense to keep it as is for now (acknowleding that it only works well in development cases) and follow up with a more robust rpc api integration into the web client so that we can invoke the above^ ? Reason I suggest this also is because of the conversation of exposing the 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. I think we should be able to get the genesis block from the client without the need for RPC calls (i.e., the client would always have the genesis block in its store). @igamigo - let us know if that's correct. 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. It's not always in the store but if any sync was performed (or if any account was created) it would be. However, you should also be able to do I initially didn't suggest this because I thought the idea was that new accounts that were created anchored in an old epoch would not be accepted by the network, was that not correct? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let (new_account, account_seed) = match AccountBuilder::new(init_seed) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.anchor((&anchor_block).try_into().unwrap()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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. Should we remove this 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.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.account_type(account_type) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.storage_mode(storage_mode.into()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.with_component(RpoFalcon512::new(key_pair.public_key())) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.with_component(BasicWallet) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.build() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Ok(result) => result, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Err(err) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let error_message = format!("Failed to create new wallet: {:?}", err); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Err(JsValue::from_str(&error_message)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Ok((new_account, account_seed, key_pair)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,12 @@ | ||
use miden_client::auth::AuthSecretKey; | ||
use miden_objects::{account::AccountData, note::NoteFile, utils::Deserializable}; | ||
use serde_wasm_bindgen::from_value; | ||
use wasm_bindgen::prelude::*; | ||
|
||
use crate::WebClient; | ||
use super::models::account::Account; | ||
use crate::{ | ||
helpers::generate_account, models::account_storage_mode::AccountStorageMode, WebClient, | ||
}; | ||
|
||
#[wasm_bindgen] | ||
impl WebClient { | ||
|
@@ -36,6 +40,47 @@ impl WebClient { | |
} | ||
} | ||
|
||
pub async fn import_account_from_seed( | ||
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. Instead of exposing cc @igamigo what do you think? 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. Although, I would probably make it so that it fails if the note isn't public, we already have a separate function to add accounts given a full 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. As discussed offline, I'm leaning towards this logic allowing for private notes as well, since "importing" from the seed for a private account is still possible, even if its just the basic wallet gen. I'm amenable to adding this to the rust client as the base implementation if you think the rust users have this use case. It was designed primarily for web wallet extensions but happy to move the code around 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.
This is not really possible in a practical sense, because you would only be able to grind for an initial account state. The moment the account's nonce increases (ie, there was a state transition registered in the network), you would not be able to reconstruct the state accordingly because it would effectively be lost. 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. If I'm understanding correctly, the intent of this method is to add a public account which already exists on chain to the client. Also, we know only some limited info about the account - e.g., So, the approach is:
If this is correct, then I think we could probably split these tasks in a few different ways. One approach could be:
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. I agree with the approach, but currently, due to the fact that the account's public key is part of the account's storage you do need to provide the seed for the 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. Good point! I think we'll need to provide the public key into the To make things convenient, we could also wrap this function in a function that contain the logic for deriving public-private key pair and then calls the appropriate functions. Basically, this "outer" function could do the following:
|
||
&mut self, | ||
init_seed: Vec<u8>, | ||
storage_mode: &AccountStorageMode, | ||
mutable: bool, | ||
) -> Result<Account, JsValue> { | ||
let client = self.get_mut_inner().ok_or(JsValue::from_str("Client not initialized"))?; | ||
|
||
let (generated_acct, account_seed, key_pair) = | ||
generate_account(client, storage_mode, mutable, Some(init_seed)).await?; | ||
|
||
if storage_mode.is_public() { | ||
// If public, fetch the data from chain | ||
let account_details = | ||
client.get_account_details(generated_acct.id()).await.map_err(|err| { | ||
JsValue::from_str(&format!("Failed to get account details: {}", err)) | ||
})?; | ||
|
||
let on_chain_account = account_details | ||
.account() | ||
.ok_or(JsValue::from_str("Account not found on chain"))?; | ||
|
||
client | ||
.add_account(on_chain_account, None, &AuthSecretKey::RpoFalcon512(key_pair), false) | ||
.await | ||
.map_err(|err| JsValue::from_str(&format!("Failed to import account: {:?}", err))) | ||
.map(|_| on_chain_account.into()) | ||
} else { | ||
// Simply re-generate the account and insert it, without fetching any data | ||
Comment on lines
+70
to
+71
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. While this works for accounts that have not been registered in the network, as mentioned in my other comment, any account that is not new would not be usable because of the fact that the hash of its latest state would not be the same as the one on the network. 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, this would not work for private accounts. I think the scope of this function should be for public accounts only as private accounts need to be imported as full |
||
client | ||
.add_account( | ||
&generated_acct, | ||
Some(account_seed), | ||
&AuthSecretKey::RpoFalcon512(key_pair), | ||
false, | ||
) | ||
.await | ||
.map_err(|err| JsValue::from_str(&format!("Failed to import account: {:?}", err))) | ||
.map(|_| generated_acct.into()) | ||
} | ||
} | ||
pub async fn import_note(&mut self, note_bytes: JsValue) -> Result<JsValue, JsValue> { | ||
if let Some(client) = self.get_mut_inner() { | ||
let note_bytes_result: Vec<u8> = from_value(note_bytes).unwrap(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
import { expect } from "chai"; | ||
import { testingPage } from "./mocha.global.setup.mjs"; | ||
import { | ||
clearStore, | ||
createNewFaucet, | ||
createNewWallet, | ||
fundAccountFromFaucet, | ||
getAccountBalance, | ||
StorageMode, | ||
} from "./webClientTestUtils"; | ||
|
||
const importWalletFromSeed = async ( | ||
walletSeed: Uint8Array, | ||
storageMode: StorageMode, | ||
mutable: boolean | ||
) => { | ||
const serializedWalletSeed = Array.from(walletSeed); | ||
return await testingPage.evaluate( | ||
async (_serializedWalletSeed, _storageMode, _mutable) => { | ||
const client = window.client; | ||
const _walletSeed = new Uint8Array(_serializedWalletSeed); | ||
|
||
const accountStorageMode = | ||
_storageMode === "private" | ||
? window.AccountStorageMode.private() | ||
: window.AccountStorageMode.public(); | ||
|
||
await client.import_account_from_seed( | ||
_walletSeed, | ||
accountStorageMode, | ||
_mutable | ||
); | ||
}, | ||
serializedWalletSeed, | ||
storageMode, | ||
mutable | ||
); | ||
}; | ||
|
||
describe("import from seed", () => { | ||
it("should import same public account from seed", async () => { | ||
const walletSeed = new Uint8Array(32); | ||
crypto.getRandomValues(walletSeed); | ||
|
||
const mutable = false; | ||
const storageMode = StorageMode.PUBLIC; | ||
|
||
const initialWallet = await createNewWallet({ | ||
storageMode, | ||
mutable, | ||
walletSeed, | ||
}); | ||
const faucet = await createNewFaucet(); | ||
|
||
const result = await fundAccountFromFaucet(initialWallet.id, faucet.id); | ||
const initialBalance = result.targetAccountBalanace; | ||
|
||
// Deleting the account | ||
await clearStore(); | ||
|
||
await importWalletFromSeed(walletSeed, storageMode, mutable); | ||
|
||
const restoredBalance = await getAccountBalance( | ||
initialWallet.id, | ||
faucet.id | ||
); | ||
|
||
expect(restoredBalance.toString()).to.equal(initialBalance); | ||
}); | ||
}); |
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.
very minor nit:
sdk
->SDK