feat: Adding import from seed for wallet recovery web client #710

julian-demox · 2025-01-30T18:19:01Z

No description provided.

crates/rust-client/src/account.rs

SantiagoPittella · 2025-02-03T14:21:16Z

crates/rust-client/src/rpc/domain/account.rs

@@ -51,6 +51,13 @@ impl AccountDetails {
            Self::Private(_, summary) | Self::Public(_, summary) => summary.hash,
        }
    }
+
+    pub fn account(&self) -> Option<&Account> {


This method should be documented

Agreed, let's document this and the above hash() method, now that AccountDetails is becoming more public as part of the new client API

crates/web-client/src/import.rs

tomyrd · 2025-02-03T15:15:09Z

crates/web-client/src/import.rs

@@ -36,6 +41,67 @@ impl WebClient {
        }
    }

+    pub async fn import_account_from_seed(


Instead of exposing Client::get_account_details and the using it here, maybe we can add expose this function as Client::import_account_from_seed and then directly calling it from the web

cc @igamigo what do you think?

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 Account object (Client::add_account).

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

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.

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.

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., init_seed, storage_mode, and is_mutable), but we don't know the account's ID.

So, the approach is:

Use init_seed, storage_mode, and is_mutable to get the initial state of the account and derive account_id from that.

Use this account_id to download the account state from the chain.

If this is correct, then I think we could probably split these tasks in a few different ways. One approach could be:

We add a method to the Client to add a public account by its ID. This may be a good function to have on the client for other purposes as well.

We encapsulate the code that derives the ID from the seed etc. into a helper function. Ideally, this would be a "pure" function (i.e., not a part of the WebClient interface) that would be used specifically by the wallet.

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 SecretKey generation as well in order to get the correct ID. So you either need to handle 2 seeds or assume the seed is the same for both things (which might be implied the approach you explained).

Good point! I think we'll need to provide the public key into the build_wallet() function as well.

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:

Take the user's seed as a parameter.

Derive the key pair for the Falcon signature.

Derive the account seed.

Get the genesis block header.

Call build_wallet() to compute the account ID.

Call Client::add_public_account() to retrieve the account from the chain.

tomyrd · 2025-02-03T15:30:40Z

crates/web-client/src/import.rs

+                        match client
+                            .add_account(
+                                account,
+                                Some(account_seed),


This parameter should be None. We only give the seed when the account is new but in this case it is already tracked by the node.

igamigo

Looks good overall! Leaving some comments for now. Most of them are minor, but there are a couple more related to the functionality of the web client/wallet and the network.

Something worth mentioning here is that it seems that the same seed is used to initialize both the client's RNG and the AccountBuilder's RNG. This is fine as long as this is the convention on the web client (ie, all account generation methods use the same seed for both; otherwise the import would fail afterward), but in reality they could be two different seeds. In more practical terms what happens is that the client's seed flows into the RNG which causes the account's SecretKey to be deterministic, but the account builder is independent.

igamigo · 2025-02-05T03:44:32Z

crates/rust-client/src/account.rs

@@ -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.


nit: rpc -> RPC (also applies to the line under the errors section)

igamigo · 2025-02-05T03:45:46Z

crates/rust-client/src/rpc/domain/account.rs

@@ -51,6 +51,13 @@ impl AccountDetails {
            Self::Private(_, summary) | Self::Public(_, summary) => summary.hash,
        }
    }
+
+    pub fn account(&self) -> Option<&Account> {


Agreed, let's document this and the above hash() method, now that AccountDetails is becoming more public as part of the new client API

igamigo · 2025-02-05T03:48:50Z

crates/web-client/src/import.rs

@@ -36,6 +41,67 @@ impl WebClient {
        }
    }

+    pub async fn import_account_from_seed(


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.

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.

igamigo · 2025-02-05T03:52:34Z

tests/integration/main.rs

@@ -1459,3 +1459,27 @@ async fn test_expired_transaction_fails() {

    assert!(submited_tx_result.is_err());
 }
+
+#[tokio::test]
+async fn test_get_account_details() {


For testing the new API, there might not be any real need to create 2 accounts/transactions here. For example, you could just create a faucet account that mints a note for a random Account ID, wait for the transaction to be included and then check the API against the local state. We want to reduce the amount of transactions we execute as much as possible to minimize impact on CI times (which unfortunately are too long already). Issue #506 has some more guidelines for this although I don't think most of them apply.

igamigo · 2025-02-05T03:56:24Z

crates/web-client/test/new_account.test.ts

+        mutable: false,
+        clientSeed,
+        isolatedClient: true,
+      })
    ).to.be.rejectedWith(/Failed to insert new wallet: AccountAlreadyTracked/);
  });
 });

 // new_faucet tests


minor nit: These are conventionally written in all caps (ie, NEW FAUCET TESTS). Same would apply for the new wallet tests.

igamigo · 2025-02-05T04:01:07Z

crates/web-client/src/helpers.rs

+    let account_type = if mutable {
+        AccountType::RegularAccountUpdatableCode
+    } else {
+        AccountType::RegularAccountImmutableCode
+    };


Should this be generalized for any AccountType? I'm guessing for the wallet you are mostly interested in doing wallets, but it would be good to generalize it and remove the mutable: bool from the function signature in favor of an account type parameter.

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

igamigo · 2025-02-05T04:02:04Z

CHANGELOG.md

@@ -2,6 +2,10 @@

 ## 0.8.0 (TBD)

+### Features 
+
+* Added wallet generation from seed & import from seed on web sdk (#710)


very minor nit: sdk-> SDK

igamigo · 2025-02-05T04:09:57Z

crates/web-client/src/helpers.rs

+        AccountType::RegularAccountImmutableCode
+    };
+
+    let anchor_block = client.get_latest_epoch_block().await.unwrap();


We should add a TODO or NOTE on import_account_from_seed() to mention the fact that this will work only as long as the account was created in the same epoch in which the network currently is.
So this will probably work for development environments, but not for all cases. Although I believe for a single seed, trying all possible epochs would not be prohibitive (at least in terms of grinding the account ID exclusively).

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.

This makes sense. Looking at examples for getting the genesis block header, I see that the existing way is to query the rpc api:

let (genesis_block, _) = self .rpc_api .get_block_header_by_number(Some(BlockNumber::GENESIS), false) .await?;

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 get_account_details call on the rust client to get the on-chain account. That wouldn't be necessary if the web client has access to the rpc api struct throughout.

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.

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 client.get_epoch_block_number(BlockNumber::GENESIS) to retrieve the block header for the first epoch. If for some reason the first block is not on the store, this would also retrieve it and store it for later use.

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?

igamigo · 2025-02-05T04:10:28Z

crates/web-client/src/helpers.rs

+    let anchor_block = client.get_latest_epoch_block().await.unwrap();
+
+    let (new_account, account_seed) = match AccountBuilder::new(init_seed)
+        .anchor((&anchor_block).try_into().unwrap())


Should we remove this unwrap()?

.try_into yields a result. My understanding of rust is still improving, but is there an alternative / can we just pass in the result in this case?

igamigo · 2025-02-05T04:15:19Z

crates/web-client/src/import.rs

+        } else {
+            // Simply re-generate the account and insert it, without fetching any data


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.

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 Account objects.

bobbinth · 2025-02-05T09:56:54Z

crates/rust-client/src/account.rs

+    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)
+    }


If we add something like add_public_account(account_id) method to the client, this method would probably not be needed.

bobbinth · 2025-02-05T10:10:41Z

crates/web-client/src/helpers.rs

+pub async fn generate_account(
+    client: &mut Client<RpoRandomCoin>,
+    storage_mode: &AccountStorageMode,
+    mutable: bool,
+    seed: Option<Vec<u8>>,
+) -> Result<(Account, [Felt; 4], SecretKey), JsValue> {


It seems like this function is specifically for generating wallet accounts. If so, I would probably name this generate_wallet() or maybe build_wallet().

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).

julian-demox added 3 commits January 30, 2025 12:09

feat: Adding import from seed for wallet recovery web client

e776b7c

changing tests around

3673e7f

changelog

6e34096

julian-demox force-pushed the julian-demox branch from 7a19387 to 6e34096 Compare January 30, 2025 19:37

linting

4d280d3

julian-demox marked this pull request as ready for review January 30, 2025 19:39

julian-demox added 4 commits January 30, 2025 13:57

linting + integrating with tip of next

b962c76

clippy-wasm

af13505

bugfix with seeds + memory issues

77caf0d

improving return type

81494e8

SantiagoPittella reviewed Feb 3, 2025

View reviewed changes

crates/rust-client/src/account.rs Outdated Show resolved Hide resolved

SantiagoPittella reviewed Feb 3, 2025

View reviewed changes

tomyrd reviewed Feb 3, 2025

View reviewed changes

cleanup + feedback

fb9c23d

igamigo reviewed Feb 5, 2025

View reviewed changes

bobbinth reviewed Feb 5, 2025

View reviewed changes

This was referenced Feb 5, 2025

Fetching Public Account State on Testnet Using the WebClient #716

Open

Create a client API for importing public accounts from the network #722

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adding import from seed for wallet recovery web client #710

feat: Adding import from seed for wallet recovery web client #710

julian-demox commented Jan 30, 2025

SantiagoPittella Feb 3, 2025

igamigo Feb 5, 2025

tomyrd Feb 3, 2025 •

edited

Loading

tomyrd Feb 3, 2025

julian-demox Feb 3, 2025

igamigo Feb 5, 2025

bobbinth Feb 5, 2025

igamigo Feb 5, 2025 •

edited

Loading

bobbinth Feb 5, 2025

tomyrd Feb 3, 2025

igamigo left a comment

igamigo Feb 5, 2025

igamigo Feb 5, 2025

igamigo Feb 5, 2025

igamigo Feb 5, 2025

igamigo Feb 5, 2025

igamigo Feb 5, 2025

julian-demox Feb 5, 2025

igamigo Feb 5, 2025

igamigo Feb 5, 2025

bobbinth Feb 5, 2025

julian-demox Feb 5, 2025

bobbinth Feb 5, 2025

igamigo Feb 5, 2025

igamigo Feb 5, 2025

julian-demox Feb 5, 2025

igamigo Feb 5, 2025

bobbinth Feb 5, 2025

bobbinth Feb 5, 2025

bobbinth Feb 5, 2025

		} else {
		// Simply re-generate the account and insert it, without fetching any data

feat: Adding import from seed for wallet recovery web client #710

Are you sure you want to change the base?

feat: Adding import from seed for wallet recovery web client #710

Conversation

julian-demox commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomyrd Feb 3, 2025 • 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

igamigo Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo left a 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

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

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

tomyrd Feb 3, 2025 •

edited

Loading

igamigo Feb 5, 2025 •

edited

Loading