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

Change Transaction/PendingTransaction behaviors #1480

Merged
merged 12 commits into from
Mar 5, 2024
Merged

Conversation

MartinMinkov
Copy link
Contributor

Description

Change the default behavior of Transaction.send() to throw an error if any errors are detected during processing. Additionally, we change isSuccess from Transaction and use status instead to indicate the transaction's current status. status can either be pending or rejected.

Also changes the tests and documentation.

…er readability and consistency with other method names
…cess boolean with status enum for better clarity
… to improve robustness of the transaction flow test
…r transaction methods

- Refactor send() method to throw an error when transaction submission fails, providing more explicit error handling.
- Update send() method documentation to reflect changes and provide better usage examples.
- Refactor safeSend() method to return a RejectedTransaction when internal errors are detected, providing more detailed feedback.
- Update safeSend() method documentation to reflect changes and provide better usage examples.
- Update wait() method usage examples in IncludedTransaction and RejectedTransaction to demonstrate error handling.
@MartinMinkov MartinMinkov marked this pull request as ready for review March 5, 2024 18:14
…action.safeWait() methods to handle transaction errors

refactor(CHANGELOG.md): rename Transaction.isSuccess to Transaction.status for better transaction state representation
@MartinMinkov MartinMinkov changed the title Change Transaction/PendingTransactio behaviors Change Transaction/PendingTransaction behaviors Mar 5, 2024
@shimkiv
Copy link
Member

shimkiv commented Mar 5, 2024

@MartinMinkov should we maybe also change the https://github.com/o1-labs/o1js/blob/feat/safe-send/src/examples/zkapps/hello-world/run-live.ts in order to check txn status instead of pendingTx.hash !== undefined? Maybe also use safeSend/Wait

@MartinMinkov
Copy link
Contributor Author

@MartinMinkov should we maybe also change the https://github.com/o1-labs/o1js/blob/feat/safe-send/src/examples/zkapps/hello-world/run-live.ts in order to check txn status instead of pendingTx.hash !== undefined?

Great callout! I'll run through all the examples for usage of .hash to change that. We'll need to change the zkapp-cli PR you made as well, if you don't mind!

…d of hash for better transaction tracking

This change will allow us to track the transaction status more accurately and handle it accordingly.
@shimkiv
Copy link
Member

shimkiv commented Mar 5, 2024

Thank you, if you don't have time I can take care of that zkapp-cli PR once o1js released, no worries.

…ng error to allow further processing of the transaction status
@MartinMinkov MartinMinkov self-assigned this Mar 5, 2024
@MartinMinkov MartinMinkov requested a review from nicc March 5, 2024 19:11
CHANGELOG.md Outdated
@@ -19,6 +19,9 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm

### Breaking changes

- Improved on parity fix of `Mina.Localblockchain` and`Mina.Network` to make `.send()` and `.wait()` throw an error by default if the transaction was not successful. https://github.com/o1-labs/o1js/pull/1480
- Changed `Transaction.isSuccess` to `Transaction.status` to better represent the state of a transaction.
- `Transaction.safeSend()` and `PendingTransaction.safeWait()` are introduced to return a `IncludedTransaction` or `RejectedTransaction` object without throwing errors.
Copy link
Member

Choose a reason for hiding this comment

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

since both of them are in the same release, it would be nice to collapse that changelog with the changes from #1422 mentioned below

for example not have both

  • .send() no longer throws an error ...
  • .send() throws an error by default ...

but just

  • .send() when using Mina.Network is now consistent with LocalBlockchain in that it throws an error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great point! done!

@MartinMinkov MartinMinkov merged commit 07e5ad8 into main Mar 5, 2024
13 checks passed
@MartinMinkov MartinMinkov deleted the feat/safe-send branch March 5, 2024 22:44
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.

None yet

4 participants