-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add WebStore to miden-client #402
Conversation
use wasm_bindgen_futures::*; | ||
|
||
// Account IndexedDB Operations | ||
#[wasm_bindgen(module = "/src/store/web_store/js/accounts.js")] |
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.
All of the js_bindings.rs
files follow the same pattern, so I will write a few comments on this first file to point out a few things and it should apply to the rest.
Here, we're basically pointing to a .js
file that implements all of the methods below. These js files will be responsible for storing or retrieving data from IndexedDB, and return a promise.
extern "C" { | ||
// GETS | ||
// ================================================================================================ | ||
#[wasm_bindgen(js_name = getAccountIds)] |
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.
By adding the module =
line above, this allows us to specify the name of the function to be executed when the corresponding rust function gets called
id: String, | ||
code_root: String, | ||
storage_root: String, | ||
vault_root: String, | ||
nonce: String, | ||
committed: bool, | ||
account_seed: Option<Vec<u8>>, |
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.
For the insert statements, I tried breaking down the information needed to be stored into simple, primitive types that can cross the rust/wasm boundary easily.
As a result, there is some difference with how some tables store things in SQLite vs. how I have it set up to be stored in IndexedDB. I'm assuming these interfaces will be refined as the codebase progresses, but think this is a good place to start.
src/store/web_store/accounts/mod.rs
Outdated
let promise = idxdb_get_account_ids(); | ||
let js_value = JsFuture::from(promise).await.unwrap(); | ||
let account_ids_as_strings: Vec<String> = from_value(js_value).unwrap(); | ||
|
||
let native_account_ids: Vec<AccountId> = account_ids_as_strings | ||
.into_iter() | ||
.map(|id| AccountId::from_hex(&id).unwrap()) | ||
.collect(); | ||
|
||
Ok(native_account_ids) |
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.
Most GET methods follow this pattern:
- Call indexed db method which returns a promise
- Call
JsFuture::from(promise).await
to get the returnedJsValue
from the indexedDB call - use
serde_wasm_bindgen::from_value
to deserialize theJsValue
into a Rust data structure.
Sometimes, there also needs to be some serializing before the indexedDB call is made to be able to pass the information down to JS (i.e. AccountID -> String representation of an AccountId). These parts of the code closely resemble/follow the serialize_{some_data}
functions in the sqlite_store
folder to prepare the data to be stored in SQLite.
Additionally, sometimes there needs to be some extra work after the indexedDB call is made and the JsValue
is parsed back into rust to construct whatever the return value needs to be. These parts of the code closely resemble/follow the parse_{some_data}
functions in the sqlite_store
folder to reconstruct the native types once the stored data is retrieved.
src/store/web_store/accounts/mod.rs
Outdated
insert_account_code(account.code()).await.unwrap(); | ||
|
||
insert_account_storage(account.storage()).await.unwrap(); | ||
|
||
insert_account_asset_vault(account.vault()).await.unwrap(); | ||
|
||
insert_account_record(account, account_seed).await.unwrap(); | ||
|
||
insert_account_auth(account.id(), auth_info).await.unwrap(); |
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.
One thing to note here (and in some other places) is that right now these inserts are not atomic. With Rust + SQLite, you can create a tx
object and interact with the DB multiple times, then call commit
on the tx
object so that it all happens atomically. In order to do this, I need to pass all the relevant data to one function in indexedDB, then can do something similar once I'm in the JS. I don't do this here yet, but I do this in other places (sorry for the inconsistency).
On my TODO list is to audit places like this where the updates are not atomic and refactor a bit to make it so.
src/store/web_store/accounts/mod.rs
Outdated
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.
Just want to point out here and throughout that my error handling when interacting with the indexedDB store is abysmal right now. I hope to add solid error handling in a following PR.
src/store/web_store/accounts/mod.rs
Outdated
/// Fetches an [AuthSecretKey] by a public key represented by a [Word] and caches it in the store. | ||
/// This is used in the web_client so adding this to ignore the dead code warning. | ||
#[allow(dead_code)] | ||
pub(crate) async fn fetch_and_cache_account_auth_by_pub_key( |
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.
This function is the only one implemented that is not defined in the Store
, but is the solution we have for the issue discussed here 0xPolygonMiden/miden-base#705.
use serde::{de::Error, Deserialize, Deserializer, Serialize}; | ||
|
||
#[derive(Serialize, Deserialize)] | ||
pub struct AccountCodeIdxdbObject { |
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.
For the GET functions, I usually wrap the data needed to be returned in an object and use serde for serialization/deserialization.
pub struct AccountCodeIdxdbObject { | ||
pub root: String, | ||
pub procedures: String, | ||
#[serde(deserialize_with = "base64_to_vec_u8_required", default)] |
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.
This was my solution to deserializing Vec<u8>
. Plan to revisit to see if there's anything easier or better, but this works well for now.
pub account_seed: Option<Vec<u8>>, | ||
} | ||
|
||
fn base64_to_vec_u8_required<'de, D>(deserializer: D) -> Result<Vec<u8>, D::Error> |
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.
Probably should put these two in a shared place. I can probably do that in this PR.
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.
In all my utils.rs
files, I moved some code out of the main mod.rs
file to make them easier to sift through. The splits I settled on feel natural to me, but open to dropping all of the utils code across the different mod.rs
s if we prefer all the logic in one place.
src/store/web_store/js/accounts.js
Outdated
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.
Across these .js
files, I basically tried my best to follow as closely along to the schema set by the sqlite store as well as the queries. Obviously there's going to be some minor differences in approaches to storing some pieces of data, but for the most part if you read a query in sqlite and look for its counter part here in an indexedDB js file, they should read mostly the same.
src/store/web_store/sync/mod.rs
Outdated
update_account(&account.clone()).await.unwrap(); | ||
} | ||
|
||
let promise = idxdb_apply_state_sync( |
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.
This is one of those places where I attempted to make this update atomic by sending all the info needed to the JS side, and making the queries atomic there. If you follow the code all the way to sync.js
in the function applyStateSync
, you can see how I tried to make all the different updates under a single transaction.
Left some TODO's here because those updates happen independently for now. Need to lop them into this call eventually or figure out a different strategy in general.
src/store/web_store/js/sync.js
Outdated
return db.transaction('rw', stateSync, inputNotes, outputNotes, transactions, blockHeaders, chainMmrNodes, async (tx) => { | ||
await updateSyncHeight(tx, blockNum); | ||
await updateSpentNotes(tx, nullifierBlockNums, nullifiers); | ||
await updateBlockHeader(tx, blockNum, blockHeader, chainMmrPeaks, hasClientNotes); | ||
await updateChainMmrNodes(tx, nodeIndices, nodes); | ||
await updateCommittedNotes(tx, outputNoteIds, outputNoteInclusionProofs, inputNoteIds, inputNoteInluclusionProofs, inputeNoteMetadatas); | ||
await updateCommittedTransactions(tx, transactionBlockNums, transactionIds); |
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.
This is how you would go about doing multiple updates across multiple indexedDB stores atomically under one transaction.
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 cool! Leaving some comments for now, going to resume looking into the PR later. In general I think the code looks good even though I'm not too familiar with IndexDB and its workflows.
let promise = idxdb_get_block_headers(formatted_block_numbers_list); | ||
let js_value = JsFuture::from(promise).await.unwrap(); | ||
let block_headers_idxdb: Vec<Option<BlockHeaderIdxdbObject>> = | ||
from_value(js_value).unwrap(); |
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.
Definitely not for this PR but it seems these could be generalized and collapsed into a function since this pattern occurs frequently.
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.
Yes, good call! Will note that as a good thing to cleanup soon after merging this 👍
src/store/web_store/js/accounts.js
Outdated
if (mostRecentRecord.accountSeed) { | ||
// Ensure accountSeed is processed as a Uint8Array and converted to Base64 | ||
let accountSeedArrayBuffer = await mostRecentRecord.accountSeed.arrayBuffer(); | ||
let accountSeedArray = new Uint8Array(accountSeedArrayBuffer); | ||
accountSeedBase64 = uint8ArrayToBase64(accountSeedArray); | ||
} |
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.
What's the limitation behind returning a base64 string as opposed to just returning a byte array? Are they serialized as strings and so this allows to compress data better?
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.
Great question. I did it this way here and in multiple places mainly because passing a Uint8Array
from JS to Rust via the WASM boundary wasn't working. So my approach here was to convert the Uint8Array
to a base64
encoded string, pass that through the WASM boundary to rust, and then do something like this
#[derive(Serialize, Deserialize)]
pub struct AccountRecordIdxdbOjbect {
pub id: String,
pub nonce: String,
pub vault_root: String,
pub storage_root: String,
pub code_root: String,
#[serde(deserialize_with = "base64_to_vec_u8_optional", default)]
pub account_seed: Option<Vec<u8>>,
}
fn base64_to_vec_u8_optional<'de, D>(deserializer: D) -> Result<Option<Vec<u8>>, D::Error>
where
D: Deserializer<'de>,
{
let base64_str: Option<String> = Option::deserialize(deserializer)?;
match base64_str {
Some(str) => base64_decode(&str)
.map(Some)
.map_err(|e| Error::custom(format!("Base64 decode error: {}", e))),
None => Ok(None),
}
}
to convert the base64
encoded string to a Rust Vec<u8>
when use serde_wasm_bindgen::from_value;
is called on the object returned from the JS function.
Doing a bit more research, I think you may be right and I think we may be able to return a Uint8Array
directly and do something like this instead
#[derive(Serialize, Deserialize)]
pub struct AccountRecordIdxdbObject {
pub id: String,
pub nonce: String,
pub vault_root: String,
pub storage_root: String,
pub code_root: String,
#[serde(deserialize_with = "deserialize_uint_8_array", default)]
pub account_seed: Option<Vec<u8>>,
}
fn deserialize_uint_8_array<'de, D>(deserializer: D) -> Result<Option<Vec<u8>>, D::Error>
where
D: Deserializer<'de>,
{
let js_value: JsValue = Deserialize::deserialize(deserializer)?;
if js_value.is_null() || js_value.is_undefined() {
return Ok(None);
}
let uint8_array = Uint8Array::from(js_value);
let vec = uint8_array.to_vec();
Ok(Some(vec))
}
to handle the deserialization properly. I think for now, I'll leave this since I know it works, and then in a follow up experiment with the other approach to see if it's more performant 👍
src/store/web_store/js/accounts.js
Outdated
try { | ||
let accountSeedBlob = null; | ||
if (account_seed) { | ||
accountSeedBlob = new Blob([new Uint8Array(account_seed)]); |
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.
Seems like the seed is returned as a Blob
here instead of a base64 string, is there a reason why they differ? If not we might want to track a work item to make them consistent
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.
Yeah, good call out. My thought process here was that anything that could be stored as a Blob
in IndexedDB probably should be since it's more performant in terms of processing and storage.
That being said, the lack of consistency when inserting into indexedDB as a Blob
and needing to return back to Rust as a base64
is a good argument for just standardizing the insert and retrieval data type.
Additionally, there are some places where I'm also forced to insert as a base64 encoded string because I think indexedDB doesn't like matching Blob
types when trying to do a lookup. So for certain tables that I know I'm going to have to look up based on a particular field, I also break this pattern and insert as a base64 string. I'm hesitant to change this now given that auditing the code and standardizing will take a bit of time and testing, but will do that quickly after this gets merged.
src/store/web_store/notes/utils.rs
Outdated
Ok(()) | ||
} | ||
|
||
pub(crate) fn serialize_output_note( |
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 wonder if these util functions can (and should!) be shared between this and SqliteStore
.
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.
Yeah I agree that there is quite a good amount of code duplication here that should be looked at more closely. There are some places in the serialization process (and parsing) that differ, but can likely work around it in the name of DRYing some code up. The things I'm thinking of here mainly are when u64
s are stored directly into the SQLite DB for things like IDs or other values. Because WASM doesn't natively support u64
s, I convert these values to String
s in the serialization process and then handle the String
s returned from WASM accordingly during deserialization (i.e. converting back to u64
or other native objects using from_hex
or something similar. I'll make sure to add this as part of cleanup tasks 👍
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.
Awesome work! I left some small comments / questions
It'd be nice if the comments describing the flow in the web store were added as doc comments, it'll probably ease it a bit for new contributors. if you plan to do a follow-up PR with documentation you can include it there.
Also one minor nit regarding note / transaction statuses is whether we should define them as constants instead of using the plain string, mostly to avoid typo errors.
Cargo.toml
Outdated
@@ -30,13 +30,15 @@ async = ["miden-tx/async"] | |||
concurrent = ["miden-lib/concurrent", "miden-objects/concurrent", "miden-tx/concurrent", "std"] | |||
default = ["std"] | |||
executable = ["std", "sqlite", "tonic", "dep:clap", "dep:comfy-table", "dep:figment", "dep:tokio", "dep:toml"] | |||
idxdb = ["dep:base64", "dep:serde-wasm-bindgen", "dep:wasm-bindgen", "dep:wasm-bindgen-futures"] |
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.
should we enforce async
feature with idxdb?
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.
Yep - seems to me that async
feature should be required for idxdb
feature.
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.
Good call!
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.
Oh, we should also update the build-wasm
target in the Makefile
to include the idxdb feature flag there
) -> Result<BTreeMap<InOrderIndex, Digest>, StoreError> { | ||
match filter { | ||
ChainMmrNodeFilter::All => { | ||
let promise = idxdb_get_chain_mmr_nodes_all(); |
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.
besides this line, isn't the rest the same for ::All
and ::List
variants?
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.
Good catch! Created a function in the utils.rs
file for chain_data
to DRY this code up a bit. In general, I'll be looking for other places with duplicated logic, etc. in a future PR!
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.
another way without creating a separate function could be to pattern match only for the promis. So you'd end up with:
let promise = match filter {
ChainMmrNodeFilter::All => idxdb_get_chain_mmr_nodes_all(),
ChainMmrNodeFilter::List(ids) => {
let formatted_list = ...;
idxdb_get_chain_mmr_nodes(formatted_list)
}
}
let js_value = JsValue::from(promise).await.unwrap():
// The rest of the code goes here
...
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.
If you think the same logic will be used in other places I'm ok with moving it into a utils module though
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.
Haven't finished reading everything yet so it's not a complete review but I left a few comments.
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.
Great work! Thank you! I only did a fairly high-level review and left a few small comments and suggestions for the future.
Overall, after some of the simpler comments above are addressed, we can merge, and address bigger comments/refactorings/code de-duplication in a follow-up PR.
Cargo.toml
Outdated
@@ -52,12 +54,15 @@ rusqlite = { version = "0.30.0", features = ["vtab", "array", "bundled"], option | |||
rusqlite_migration = { version = "1.0", optional = true } | |||
serde = { version = "1.0", features = ["derive"] } | |||
serde_json = { version = "1.0", features = ["raw_value"] } | |||
serde-wasm-bindgen = { version = "0.6.5", optional = true } |
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.
nit: we usually don't specify patch version (e.g., we'd use 0.6
instead of 0.6.5
).
Though, I see that some other dependencies here also have patch versions specified. @igamigo we should fix that in a different PR.
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.
Fixed!
Cargo.toml
Outdated
@@ -30,13 +30,15 @@ async = ["miden-tx/async"] | |||
concurrent = ["miden-lib/concurrent", "miden-objects/concurrent", "miden-tx/concurrent", "std"] | |||
default = ["std"] | |||
executable = ["std", "sqlite", "tonic", "dep:clap", "dep:comfy-table", "dep:figment", "dep:tokio", "dep:toml"] | |||
idxdb = ["dep:base64", "dep:serde-wasm-bindgen", "dep:wasm-bindgen", "dep:wasm-bindgen-futures"] |
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.
Yep - seems to me that async
feature should be required for idxdb
feature.
type SerializedTransactionData = ( | ||
String, | ||
String, | ||
String, | ||
String, | ||
String, | ||
Vec<u8>, | ||
Option<Vec<u8>>, | ||
Option<Vec<u8>>, | ||
Option<String>, | ||
String, | ||
Option<String>, | ||
); |
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.
It might be safer to use a struct here and in other similar places - e.g., something like:
pub struct SerializedTransactionData {
pub transaction_id: String,
pub account_id: String,
...
};
Instantiating and destructuring such structs should be nearly as easy as tuple types - but it would make mixing up fields more difficult.
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.
Agreed! Didn't like the type. Changed it across WebStore
, but might also want to something similar across SqliteStore
👍
pub(super) fn serialize_transaction_data( | ||
transaction_result: TransactionResult, | ||
) -> Result<SerializedTransactionData, StoreError> { |
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.
Conversions like this could be replaced with TryInto
implementations - i.e., TryInto<SerializedTransactionData> for TransactionResult
.
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.
Great point here and below. I might hold off on refactoring these mainly because I know there's quite a bit of shared logic in these serialize
functions and parse
functions between WebStore
and SqliteStore
. I'm hoping I can extract most of the shared logic into helper functions and then implement these TryInto
or TryFrom
functions for each specific store type.
pub fn parse_account_record_idxdb_object( | ||
account_stub_idxdb: AccountRecordIdxdbOjbect, | ||
) -> Result<(AccountStub, Option<Word>), StoreError> { |
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.
Similar to my other comment, this could be replaced with a TryInto
implementation.
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.
Same as above. Definitely need a refactor here, but think it's part of a larger refactor to consolidate shared logic with SqliteStore
so holding off for this PR.
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.
Nice job with this implementation! Finished my review, left some more comments.
Addressed as much feedback as I could! Just a heads up, I'm seeing this when I run
I think this also shows up in regular, non |
Running |
Fixed! Seeing this now in the integration tests
Unsure what that could be as well! |
You probably need to rebase this branch from the latest |
3d151e7
to
85a6ff1
Compare
I should be rebased I think 🤔 I tried to do this locally, but not sure if it's the same conditions given that I'm on a forked repo and there's funkiness with origin vs. upstream |
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.
LGTM!
@dagarcia7 #407 got merged, it'll probably get some conflicts when you merge/rebase |
85a6ff1
to
c7825fc
Compare
c7825fc
to
42fea20
Compare
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.
LGTM!
* Add WebStore to miden-client * Rebased off latest next * Rust docs command fix --------- Co-authored-by: Dennis Garcia <dennis@demoxlabs.xyz>
Summary
This PR adds the
WebStore
, a WASM-compatible implementation of the miden-clientStore
trait.It uses IndexedDB as its backend storage method to provide dependable, high-performance, and persistent storage for web-based clients, enabling seamless interaction with the Miden node in web contexts.
The existing
SqliteStore
utilizes SQLite as its backend, taking advantage of its robust and efficient database capabilities for data persistence. SQLite is a widely-used, serverless, self-contained SQL database engine that offers stable storage and effective querying for applications.However, SQLite is not compatible with WASM due to its reliance on native file system access and multi-threading, which are not available in the web environment. WASM operates within a restricted sandbox in the browser, lacking direct access to the underlying file system and threading capabilities that SQLite requires.