Skip to content
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

Web Client Submit Transaction Fix #760

Merged
merged 1 commit into from
Feb 26, 2025

Conversation

dagarcia7
Copy link
Collaborator

Summary

This PR consolidates the submit_transaction and submit_transaction_with_prover calls exposed via WASM in the web client to just submit_transaction.

I made this change because in #720, I tried making it so that the top-level WebClient wrapper that I introduced to allow the web workers work only exposed one submit_transaction call with an optional prover. Then, depending on whether a prover was passed, it would call into submit_transaction or submit_transaction_with_prover as both of these calls were exposed from the rust side. The result of this could be confusing for consumers of the SDK because despite trying to consolidate the calls at the top level, the Typescript typings would still indicate that submit_transaction_with_prover is callable directly.

As a result, I decided to consolidate the calls at the rust level, and fix the JS accordingly. Now there should be no confusion when calling submit_transaction as the it's the only call to make and the parameters are very clear.

@dagarcia7 dagarcia7 marked this pull request as ready for review February 25, 2025 08:36
Copy link
Collaborator

@SantiagoPittella SantiagoPittella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, left a small comment

@@ -4,6 +4,7 @@

* [BREAKING] Added Initial Web Workers Implementation to Web Client (#720).
* Web Client Fix: Handled Case Where Web Workers are Not Available (#743).
* Web Client Submit Transaction Fix: Typescript Typings Now Match Underlying Client Call (#760).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be placed in a new section named ## 0.7.2 (TBD) or similar.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok so 0.7.1 hasn't actually been patch released yet, but when I added the section in this PR, I mistakenly put a date for the 0.7.1 section because I didn't know that (TBD) was right way to do it 😅 It's also slightly confusing that I got a bit ahead of myself and published 0.7.1 of the SDK version so now I'm a version ahead in the SDK. In the future, I plan to add some automation around publishing the SDK to fall in line with when releases, patch releases, and merges to the next branch are made so that this desync doesn't happen.

Anyway, all that to say that I think the correct thing here is to keep the 0.7.1 section with a (TBD) and then put this line under this section as well 👍

@dagarcia7 dagarcia7 force-pushed the submit-transaction-fix branch from 62bab28 to a77d7a2 Compare February 26, 2025 02:26
@dagarcia7 dagarcia7 merged commit 4b72656 into 0xPolygonMiden:main Feb 26, 2025
13 checks passed
@dagarcia7 dagarcia7 deleted the submit-transaction-fix branch February 26, 2025 03:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants