feat: add submit_with_prover to web client #674
Conversation
fix: test docs: update changelog
SantiagoPittella
force-pushed
the
santiagopittella-per-transaction-proving-web-client
branch
from
f2d550f to
8947948
Compare
There was a problem hiding this comment.
Looks good! Let's also wait on @dagarcia7 or someone from the team to add their review as well before merging.
| use wasm_bindgen::prelude::*; | ||
|
|
||
| #[wasm_bindgen] | ||
| pub struct ProverWrapper { |
There was a problem hiding this comment.
I think we could just name this TransactionProver, or maybe Prover since this is the pattern from other web client structs
There was a problem hiding this comment.
Agreed! TransactionProver is nice 💯
There was a problem hiding this comment.
Looks good to me overall! Have one suggestion (though not critical) on testing this in a more isolated manner and an open question on whether this eliminates the need for the create_client to take a prover_url param anymore.
Thanks for tagging me and letting me take a look!
| use wasm_bindgen::prelude::*; | ||
|
|
||
| #[wasm_bindgen] | ||
| pub struct ProverWrapper { |
There was a problem hiding this comment.
Agreed! TransactionProver is nice 💯
|
|
||
| #[wasm_bindgen] | ||
| pub struct ProverWrapper { | ||
| prover: Arc<dyn TransactionProver>, |
There was a problem hiding this comment.
Good idea here with the Arc. Haven't run into any issues yet with the other wrappers where I would've just done something like
#[wasm_bindgen]
pub struct ProverWrapper(TransactionProver)
#[wasm_bindgen]
impl ProverWrapper {
pub fn new_local_prover() -> ProverWrapper {
ProverWrapper(LocalTransactionProver::new(Default::default()))
}
}
but I'm sure yours is safer 🤔 I might need to revisit some of the other models.
| @@ -124,6 +125,7 @@ before(async () => { | |||
| window.OutputNote = OutputNote; | |||
| window.OutputNotesArray = OutputNotesArray; | |||
| window.Rpo256 = Rpo256; | |||
| window.ProverWrapper = ProverWrapper; | |||
There was a problem hiding this comment.
Thank you (and sorry) for dealing with the pain of having to add this kind of thing to so many files. I promise I've tried to improve this by automatically detecting what objects get exported from the WASM typings, but it turned out to be super difficult. I still plan on revisit, but for now it's a huge pain
On that note, I think the only place you missed was global.test.d.ts.
There was a problem hiding this comment.
On that note, I think the only place you missed was global.test.d.ts.
Thanks!
|
|
||
| let prover = window.ProverWrapper.new_local_prover(); | ||
|
|
||
| await client.submit_transaction_with_prover(transaction_result, prover); |
There was a problem hiding this comment.
I think this is ok for now, but I think it would also be nice to have a testing section specifically for submit_transaction_with_prover where we could have one simple mint transaction test with a local prover like (instead of modifying this gnarly custom transactions test) and another with a remote prover. There's some good test infrastructure in place already so that when you run yarn test:remove_prover it sets an env variable and runs a specific set of tests. I think you could point it to this testing section.
Also, I'm assuming this removes the need for the prover_url param in the create_client call for the web client, right?
miden-client/crates/web-client/src/lib.rs
Line 58 in 2bac040
will that be removed in a future issue?
There was a problem hiding this comment.
I'm working on a test with the remote prover.
Also, I'm assuming this removes the need for the prover_url param in the create_client call for the web client, right?
The prover_url is still needed because this new method, submit_with_prover, isn't meant to be used by default. When initializing the client we set a default prover, that why we have prover_url, and the new method is for changing the proving method for a specific transaction without changing the default set in the client.
closes #604