Add Custom Transactions #519

dagarcia7 · 2024-09-10T16:43:58Z

Summary

This PR adds support for creating and executing custom transactions and notes through the web client. This was done by exposing the following functions to the web client: compile_tx_script, compile_note_script, new_transaction, and submit_transaction. Beyond just exposing these functions, some more in depth work was needed to expose interfaces to be able to work with and manipulate some of the native miden objects via the web client as well. These are found under the models directory (for now), and follow the simple pattern of exposing a struct to WASM via the wasm_bindgen attribute that wraps around a native miden struct. For now, the minimal amount of work was done to fill out the interfaces for these WASM-compatible structs, but overtime, they will grow to resemble that of their native counterparts so that users of the web client can use these objects in web much like they do in rust.

Notes

As mentioned in the summary, the work done in this PR resembles more or less the minimum amount of work needed to be able to execute and submit custom transactions. The interfaces under the models directory will mature over time, and so will the client functions exposed to the web client in their contracts.
This is the code I followed closely and wrote a very similar test in my test.html file. I was able to reproduce the success and failure case based on the modification of the transaction script.
Also included in the code are some slight fixes due to a few previous commits breaking some functionality in the client. I will make sure to highlight those via comments.

dagarcia7 · 2024-09-10T21:08:31Z

crates/web-client/test.html

@@ -451,7 +474,7 @@
            let webClient = await createMidenWebClient();
            let accountId = await createNewWallet(webClient, "OffChain", true);

-            await getAccount(webClient, accountId);
+            await getAccount(webClient, accountId.to_string());


All of these to_string()s are a result of modifying webClient.new_wallet and webClient.new_faucet to return an actual AccountId object as opposed to a simple string. As mentioned in the PR overview, I plan on revisiting those interfaces and tightening up their contracts to make them nicer to use. But I want to avoid doing too much in this PR since it already includes a lot!

dagarcia7 · 2024-09-10T21:25:46Z

crates/rust-client/src/store/web_store/accounts/mod.rs

All of these changes under web_store files are small things that were missed in previous commits that I noticed after rebasing that were causing issues with the web client. Not anyone's fault but my own given that it's difficult to test anything web-client related at the moment 😅 But hopefully with this PR we have the infrastructure to add the janky integration tests I have in test.html and add them to a proper framework and pipeline.

Yeah, this is unfortunate. I think we try to kept this code in line with the changes on the native client crate, but were quite inconsistent doing so. Tests should help out a lot in this regard.

dagarcia7 · 2024-09-10T21:48:09Z

crates/web-client/src/models/account_id.rs

The structure of most of these files under models is the same. The idea is to expose a struct to JS via wasm_bindgen that just serves as a wrapper to be able to manipulate the underlying native miden struct. As you can see in this AccountId struct implementation, this doesn't necessarily reflect all the things you should be able to do with AccountId yet. These interfaces are built so far for what's necessary to be able to make custom transactions work. Over time, they will likely become much closer to the interfaces of the underlying structs.

dagarcia7 · 2024-09-10T21:49:10Z

crates/web-client/src/models/account_id.rs

+        self.0.is_regular_account()
+    }
+
+    #[allow(clippy::inherent_to_string)]


WASM (to my knowledge) doesn't really know what to do with the Display attribute so exposing a to_string() instead for some of these which clippy doesn't like.

dagarcia7 · 2024-09-10T21:57:07Z

crates/web-client/src/models/rpo256.rs

+
+#[wasm_bindgen]
+impl Rpo256 {
+    pub fn hash_elements(felt_array: &FeltArray) -> RpoDigest {


An interesting note about wasm_bindgen is that it doesn't really like dealing with Vec<&T>, but Vec<T> does work. This creates a potential problem in JS for users because if I were to pass an array like this

let noteArguments = [new Felt("1"), new Felt("2")] let commitment = Rpo256.hash_elements(noteArguments)

then because it is passed as a Vec<Felt>, it now takes ownership of the noteArguments array and the user will get an error if they attempt to use that variable for something else.

To mitigate this, an idea I had was to create a {T}Array struct that serves as a means to be able to pass a collection of elements to/from the WASM boundary that gives the user more control over the ownership of variables. With FeltArray for example, because you're passing a reference now that holds a collection of elements, you can reuse the same variable in JS that holds this object. In the FeltArray struct itself, there are ways to pass a full collection of Felt objects which will consume the collection passed in, or you can call append with individual &Felt elements so the original collections/elements are untouched and reusable.

I still have to poke through some of these files I created and see where I can mitigate some of these ownership issues by creating {T}Array wrappers, but figured this level of control was a better experience for a user of the SDK as opposed to getting null pointer errors unintentionally any time you reuse a variable.

dagarcia7 · 2024-09-10T22:02:44Z

crates/web-client/src/models/felt.rs

+#[wasm_bindgen]
+impl FeltArray {
+    #[wasm_bindgen(constructor)]
+    pub fn new(felts_array: Option<Vec<Felt>>) -> FeltArray {


Made a comment below about the reason for a struct like FeltArray but just wanted to highlight that the new constructor fully consumes a collection of Felt elements that is passed in, but the append function allows you to build up this collection internally by passing &Felt objects so it leaves whatever variables you had set before in JS untouched and reusable.

igamigo

Looking great! Left some comments for now and I'll try to complete the review soon.

igamigo · 2024-09-11T17:08:07Z

CHANGELOG.md

@@ -9,6 +9,7 @@

 ### Features

+* Added support for custom transactions in web client


We should append the PR number to this

igamigo · 2024-09-11T17:19:52Z

crates/rust-client/src/store/web_store/js/notes.js

    ignored,
-    importedTag
+    importedTag,
+    nullifierHeight,
 ) {


Just as a heads up, this will change fairly soon as described in this discussion (WIP PR).

Thanks for the heads up! I will try and keep a close eye on it 👍

Yes, the store internal structure will be simplified quite a bit. Tomorrow I'll try and make the necessary changes in the web store but it might conflict with the changes made here.

igamigo · 2024-09-11T17:27:27Z

crates/web-client/src/models/account_id.rs

+    pub(crate) fn as_native_account_id(&self) -> NativeAccountId {
+        self.0
+    }


I think conventionally this would be called inner(), but I don't mind if we kept it named like this.

igamigo · 2024-09-11T17:28:54Z

crates/web-client/src/models/account_id.rs

It'd be great if there were some minor doc comments at the module level or even on some of the public functions explaining the reasoning behind some of the code structure (similar to what you are doing with these PR comments)

igamigo · 2024-09-11T17:40:02Z

crates/web-client/src/models/advice_map.rs

+    pub fn insert(&mut self, key: &RpoDigest, value: &FeltArray) -> AdviceMap {
+        let native_rpo_digest = key.as_native_rpo_digest();
+        let felts = value.felts_as_vec();
+        let native_felts = felts.into_iter().map(|felt| felt.as_native_felt()).collect();
+        self.0.insert(native_rpo_digest, native_felts);
+        AdviceMap(self.0.clone())
+    }


Because this takes &mut self, is there a benefit to returning the updated AdviceMap? If the structure is modified internally, then maybe it might be better to return an Option<FeltArray> or something as it is done by the internal structure (maybe a bool that indicates whether there was a value for that key already is simpler). If the idea is that the structure is modified and returned, maybe this can take a mut self.

I agree, returning the Option<Vec<Array>> directly from the self.0.insert/2 sounds like the way to go.

Makes a ton of sense! Thanks for catching 💯

igamigo · 2024-09-11T17:45:34Z

crates/web-client/src/models/felt.rs

+#[wasm_bindgen]
+impl Felt {
+    #[wasm_bindgen(constructor)]
+    pub fn new(u64_as_str: &str) -> Felt {


Is there any reason why this cannot be a u64 instead of &str?

I was hesitant to pass 64-bit numbers to rust from JS because I was worried they wouldn't be represented well, but you're right, I think with BigInt, we should be fine. I'll do some refactoring 👍

igamigo · 2024-09-11T17:52:11Z

crates/web-client/src/models/note_id.rs

+    pub(crate) fn as_native_note_id(&self) -> NativeNoteId {
+        self.0
+    }


Is it worth adding these conversions as Into<NativeNoteId> for NoteId? (and the other way around?)

Nice! Great feedback. I went ahead and changed all of these to their appropriate conversion functions. This should also take care of this comment #519 (comment).

igamigo · 2024-09-11T17:53:48Z

crates/web-client/src/models/note_type.rs

+
+#[wasm_bindgen]
+impl NoteType {
+    pub fn new_private() -> NoteType {


nit: I think I'd drop the new_ prefix from these functions (unless there is a specific reason for these)

igamigo · 2024-09-11T18:04:02Z

crates/web-client/src/models/transaction_request.rs

+    pub fn with_unauthenticated_input_notes(
+        &mut self,
+        notes: Vec<NoteAndArgs>,
+    ) -> TransactionRequest {
+        let native_note_and_note_args: Vec<(NativeNote, Option<NativeNoteArgs>)> = notes
+            .into_iter()
+            .map(|note_and_args| note_and_args.as_native_note_and_args_tuple())
+            .collect();
+        let native_transaction_request =
+            self.0.clone().with_unauthenticated_input_notes(native_note_and_note_args);
+        TransactionRequest(native_transaction_request)
+    }


I think if the Into<> trait were implemented as mentioned in the previous comment, it could simplify some conversion code (things like collect::<Vec<_>>) and maybe make it more readable, although maybe this doesn't work with the current wasm bindgen setup.

Essentially it feels like most of these APIs (like this with_unauthenticated_input_notes()) could follow this pattern, more or less:

pub(crate) fn foo(&self, wasm_object_a: WasmWrapperA, wasm_object_b: WasmWrapperB) -> WasmWrapperResult { self.inner().foo(wasm_object_a.into(), wasm_object_b).into()).into() }

Ah, yeah I see your point. The tricky thing with these methods, however, is that they need to accept a Vec (or iterator) of tuples. In this case the tuple is of (Note, Option<NoteArgs>). WASM, however, doesn't like passing tuples so I had to create this NoteAndArgs struct to make it easier to pass via the WASM boundary. So I modified this function to look like this

pub fn with_unauthenticated_input_notes( mut self, notes: Vec<NoteAndArgs>, ) -> Self { let native_note_and_note_args: Vec<(NativeNote, Option<NativeNoteArgs>)> = notes .into_iter() .map(|note_and_args| note_and_args.into()) .collect(); self.0 = self.0.with_unauthenticated_input_notes(native_note_and_note_args); self } I tried to incorporate your feedback of implementing the `<Into>` trait for the custom `NoteAndArgs` struct into its deconstructed native tuple and also changed the signature to accept `mut self` so I can chain calls on the returned `TransactionRequest` object like you can with the native struct. Let me know what you think!

Another thought I had was instead of passing a Vec<NoteAndArgs>, making a NoteAndArgsArray struct just like I did with FeltArray. That way, I can pass the array from JS as a reference, and convert it to a Vec<(NativeNote, NativeNoteArgs>) with a simple .into(). Was thinking of going through and making these {some_type}Array structs anywhere I'm passing Vec<{some_type}> to make things consistent and passable by reference.

SantiagoPittella · 2024-09-12T16:55:30Z

crates/web-client/clean.js

+      }
+    });
+  });
+});


A new line is missing.

SantiagoPittella · 2024-09-12T16:56:09Z

crates/web-client/rollup.config.js

                cargoArgs: [
+                    // "--profile", "dev", // Use the dev profile


Should this line be deleted?

SantiagoPittella · 2024-09-12T17:08:02Z

crates/web-client/src/models/advice_map.rs

+    pub fn insert(&mut self, key: &RpoDigest, value: &FeltArray) -> AdviceMap {
+        let native_rpo_digest = key.as_native_rpo_digest();
+        let felts = value.felts_as_vec();
+        let native_felts = felts.into_iter().map(|felt| felt.as_native_felt()).collect();
+        self.0.insert(native_rpo_digest, native_felts);
+        AdviceMap(self.0.clone())
+    }


I agree, returning the Option<Vec<Array>> directly from the self.0.insert/2 sounds like the way to go.

julian-demox · 2024-09-13T14:50:41Z

crates/web-client/test/mocha.global.setup.mjs

-});
-
-export { testingPage };
+import puppeteer from "puppeteer";


nit: Do we know why these diffs are showing the whole file as changed?

Actually have no idea. I have a feeling it has something to do with the fact that I started working on a Windows machine, but it's difficult to pinpoint the reason. I might do a manual diff via diff checker, check out these files again, and then try to clean them up!

Think I found the issue! Replied in a comment about it here. Good catch! 👍

tomyrd · 2024-09-12T19:21:54Z

crates/rust-client/src/store/web_store/js/notes.js

    ignored,
-    importedTag
+    importedTag,
+    nullifierHeight,
 ) {


Yes, the store internal structure will be simplified quite a bit. Tomorrow I'll try and make the necessary changes in the web store but it might conflict with the changes made here.

crates/web-client/js/types/index.d.ts

crates/web-client/src/models/fungible_asset.rs

crates/web-client/src/models/note_assets.rs

crates/web-client/src/models/note_execution_hint.rs

crates/web-client/src/transactions.rs

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

crates/web-client/test.html

tomyrd · 2024-09-13T15:05:49Z

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

Strange how Github marks this file as "changed" but the removed and added part are the same.

Some files like crates/web-client/test/webClientTestUtils.js and crates/web-client/test.html are also marked as entirely removed and then added which makes the changes harder to spot. It's maybe a Github bug.

Yeah @julian-demox noticed the same thing 🤔. I replied here and I think it has something to do with Windows but need to look into it a bit more. In the meantime I'll try to fix this so it's easier to review!

Ok should be fixed, I think. The issue was with the line endings that were getting added as a result of working on a windows machine now. Windows adds CRLF endings by default, and Macs or unix-based systems add LF by default. I inadvertently changed the line endings to some existing files on accident which is why the diff showed up as it is. I noticed that the repo has different line endings across the board for different files and is inconsistent. If we wanted to fix this issue, I think we could add a .gitattribute to ensure all line endings are of a certain type (probably LF), but for now, I just ran this command git config --global core.autocrlf false on my machine which will make it so that whenever I modify existing files and commit, it will use the existing line endings, and when I create a new file, it will use the windows ones.

crates/web-client/test.html

SantiagoPittella

looks good! thx!

igamigo

Thanks! Overall this looks good to me! I left some remaining comments/questions, the main one being about the import note API. It's unfortunate that bindgen does not do away with some of the boilerplate but in any case it's not too complicated to follow anyway.

igamigo · 2024-09-18T21:53:07Z

crates/rust-client/src/store/web_store/notes/mod.rs

+        let block_num = self.get_sync_height().await?;
+
+        insert_input_note_tx(block_num, note).await


This looks a bit weird to me. I understand this is the store API implementation, but why does the block_num need to be passed?

Oh, yeah this was just me trying to catch up with the latest changes on next. I noticed this function was updated to take a block_num here

miden-client/crates/rust-client/src/store/sqlite_store/notes.rs

Line 267 in 93bf875

insert_input_note_tx(&tx, block_num, note)?;

so I just copied that!

igamigo · 2024-09-18T21:57:02Z

crates/rust-client/src/store/web_store/notes/utils.rs

    let serialized_data = serialize_output_note(note)?;
+    let expected_height = serialized_data.expected_height.or(Some(block_num.to_string()));


Same here. It seems that the expected height is stored separately. Can this not be retrieved from its NoteStatus?

Similar story to the above comment, except I was looking more at this guy https://github.com/0xPolygonMiden/miden-client/blob/223940abbc07c482882ddf50759ab18ffdc7a368/crates/rust-client/src/store/sqlite_store/notes.rs#L373C22-L373C70

igamigo · 2024-09-18T22:05:20Z

crates/web-client/src/models/advice_map.rs

+        let felts_vec: Vec<Felt> = value.into();
+        let native_felts: Vec<NativeFelt> = felts_vec.into_iter().map(|felt| felt.into()).collect();


Because you have implemented impl From<FeltArray> for Vec<NativeFelt> could this not be let native_felts: Vec<NativeFelt> = value.into()?

igamigo · 2024-09-18T22:06:34Z

crates/web-client/src/models/felt.rs

+
+impl From<FeltArray> for Vec<NativeFelt> {
+    fn from(felt_array: FeltArray) -> Self {
+        felt_array.0.into_iter().map(|felt| felt.into()).collect()


nit: This could be written as felt_array.0.into_iter().map(NativeFelt::from).collect() (this also happens in other places throughout the PR, but in general feel free to disregard)

igamigo · 2024-09-18T22:19:01Z

crates/web-client/src/models/note_metadata.rs

+        let native_sender: NativeAccountId = sender.into();
+        let native_note_type: NativeNoteType = note_type.into();
+        let native_tag: NativeNoteTag = note_tag.into();
+        let native_execution_hint: NativeNoteExecutionHint = note_execution_hint.into();
+        let native_aux: NativeFelt = match aux {
+            Some(felt) => felt.into(),
+            None => Default::default(),
+        };
+
+        let native_note_metadata = NativeNoteMetadata::new(
+            native_sender,
+            native_note_type,
+            native_tag,
+            native_execution_hint,
+            native_aux,
+        )


I think, to simplify this, you could directly define members within new():

let native_note_metadata = NativeNoteMetadata::new( sender.into(), note_type.into(), tag.into(), note_execution_hint.into(), aux.unwrap_or_default().into(), )

igamigo · 2024-09-18T22:22:23Z

crates/web-client/src/models/transaction_script.rs

+    }
+}
+
+// Conversions


nit: Usually code region delimiters throughout Miden repos are written like this:

// CONVERSIONS // ================================================================================================

dagarcia7 commented Sep 10, 2024

View reviewed changes

dagarcia7 marked this pull request as ready for review September 10, 2024 22:03

dagarcia7 force-pushed the add-custom-transactions branch 2 times, most recently from 58c3d8c to f3eef0c Compare September 10, 2024 22:53

bobbinth requested a review from igamigo September 11, 2024 06:22

igamigo reviewed Sep 11, 2024

View reviewed changes

SantiagoPittella reviewed Sep 12, 2024

View reviewed changes

julian-demox reviewed Sep 13, 2024

View reviewed changes

tomyrd reviewed Sep 13, 2024

View reviewed changes

dagarcia7 force-pushed the add-custom-transactions branch 2 times, most recently from abba66d to 111888a Compare September 16, 2024 18:38

Dennis Garcia added 2 commits September 17, 2024 18:24

Adding Custom Notes and Custom Transactions to Web SDK

96858b9

Small fixes

91ad5a8

dagarcia7 force-pushed the add-custom-transactions branch from 111888a to 91ad5a8 Compare September 18, 2024 01:20

Lint

77b9380

SantiagoPittella approved these changes Sep 18, 2024

View reviewed changes

igamigo reviewed Sep 18, 2024

View reviewed changes

Feedback

c52cfe8

dagarcia7 merged commit b0f50cb into 0xPolygonMiden:next Sep 19, 2024
14 checks passed

dagarcia7 deleted the add-custom-transactions branch September 19, 2024 05:20

		@@ -9,6 +9,7 @@

		### Features

		* Added support for custom transactions in web client

		let block_num = self.get_sync_height().await?;

		insert_input_note_tx(block_num, note).await

		let serialized_data = serialize_output_note(note)?;
		let expected_height = serialized_data.expected_height.or(Some(block_num.to_string()));

		let felts_vec: Vec<Felt> = value.into();
		let native_felts: Vec<NativeFelt> = felts_vec.into_iter().map(\|felt\| felt.into()).collect();

Add Custom Transactions #519

Add Custom Transactions #519

Conversation

dagarcia7 commented Sep 10, 2024 • edited Loading

Summary

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igamigo Sep 11, 2024 • 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

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

igamigo Sep 11, 2024 • 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

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

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

igamigo Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagarcia7 commented Sep 10, 2024 •

edited

Loading

igamigo Sep 11, 2024 •

edited

Loading

igamigo Sep 11, 2024 •

edited

Loading

igamigo Sep 18, 2024 •

edited

Loading