Fix parity between local blockchain and network APIs #1422
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…o PendingTransaction A simple rename from `TransactionId` to `PendingTransaction`. This rename provides better clarity on what is returned after sending a transaction.
…ated functions to improve type safety
PendingTransaction adds the `data` and `errors` property in the Network version. We now specify the properties inside `PendingTransaction` by making data be return type of sendZkApp which is a `SendZkAppResponse`. Also, this fixes a current bug where the `txnId` can be undefined when executing the polling for requests.
…ta from any block at the best tip This change was made because the issue o1-labs/Archive-Node-API#7 has been resolved, making the temporary fix redundant.
Adds graphql.ts which is a module to hold all response type and graphql query resources when interacting with the mina daemon graphql. We remove these from fetch.ts as fetch was starting to get bloated and harder to read. This aims to be a refactor that offers a cleaner seperation of concerns between the graphql resources and the actual requests being sent
…at have been included in the blockchain This new type extends the PendingTransaction type with an additional 'isIncluded' boolean property. This will allow us to easily distinguish between pending and included transactions in our code.
…this type from other modules
… Omit utility type
This method sends the Transaction to the network and throws an error if internal errors are detected. This provides a more robust way of handling transaction failures.
…saction failure properly
…unt-update.js' to maintain file naming consistency
…and accountQuery from account.ts to graphql.ts
…is constants data
…SlotResponse type to fetch current slot data from the server This change allows the application to fetch the current slot data from the server, which is necessary for the application's functionality.
This commit introduces a new test suite for the zkApp transaction flow. The tests cover various scenarios including local and remote tests, transaction verification, zkApp deployment, method calls, event emission and fetching, and action rollups. The tests are designed to ensure the correct behavior of the zkApp and its interaction with the Mina protocol.
…s to increase test coverage
MartinMinkov
commented
Feb 20, 2024
src/lib/mina/mina-instance.ts
Outdated
| @@ -20,6 +35,11 @@ export { | |||
| activeInstance, | |||
| setActiveInstance, | |||
| ZkappStateLength, | |||
| reportGetAccountError, | |||
There was a problem hiding this comment.
Moved these methods here as they are shared between local-blockchain and remote
There was a problem hiding this comment.
hm, so this is not ideal, because it (re)creates a dependency cycle between this module and account-update.ts.
maybe add yet another new module with the methods you added here - something like validate-transaction.ts or transaction-validation.ts?
ymekuria
reviewed
Feb 21, 2024
src/lib/mina/mina-instance.ts
Outdated
| @@ -20,6 +35,11 @@ export { | |||
| activeInstance, | |||
| setActiveInstance, | |||
| ZkappStateLength, | |||
| reportGetAccountError, | |||
There was a problem hiding this comment.
hm, so this is not ideal, because it (re)creates a dependency cycle between this module and account-update.ts.
maybe add yet another new module with the methods you added here - something like validate-transaction.ts or transaction-validation.ts?
There was a problem hiding this comment.
it's so much cleaner with this file broken up into digestible pieces!
| @@ -0,0 +1,291 @@ | |||
| import { | |||
There was a problem hiding this comment.
great that you added this!
…ethod to hash property for better readability and simplicity
…tUpdateErrors variable
This change throws an error when a transaction is rejected, providing more detailed feedback about the transaction failure.
…jects for cleaner error handling
…a.ts to transaction.ts
…etter readability and performance fix(run-live.ts): correct the usage of hash in console.log statements to ensure accurate transaction hash display
| if (accountUpdateErrors.length === n) { | ||
| let errorsForFeePayer = errors[0]; | ||
| if (errors.length > n) { | ||
| let errorsForFeePayer = errors.shift() ?? []; |
There was a problem hiding this comment.
damn I always mix up shift and unshift
There was a problem hiding this comment.
not the best naming imo :P
mitschabaude
approved these changes
Feb 21, 2024
- Add fix for parity between `Mina.LocalBlockchain` and `Mina.Network` to ensure consistent behaviors - Include changes to `TransactionId`, `transaction.send()`, and `transaction.wait()` for better error handling and transaction state representation
…ina.ts to mina-instance.ts for better code organization and maintainability
…ction The changes related to transaction handling in `Mina.LocalBlockchain` and `Mina.Network` are significant and can potentially break existing implementations. Therefore, they have been moved from the 'Fixed' section to a new 'Breaking changes' section to highlight their importance and potential impact.
barriebyron
reviewed
Mar 4, 2024
| - Changed the `TransactionId` type to `Transaction`. Additionally added `PendingTransaction` and `RejectedTransaction` types to better represent the state of a transaction. | ||
| - `transaction.send()` no longer throws an error if the transaction was not successful for `Mina.LocalBlockchain` and `Mina.Network`. Instead, it returns a `PendingTransaction` object that contains the error. Use `transaction.sendOrThrowIfError` to throw the error if the transaction was not successful. | ||
| - `transaction.wait()` no longer throws an error if the transaction was not successful for `Mina.LocalBlockchain` and `Mina.Network`. Instead, it returns either a `IncludedTransaction` or `RejectedTransaction`. Use `transaction.waitOrThrowIfError` to throw the error if the transaction was not successful. | ||
| - `transaction.hash()` is no longer a function, it is now a property that returns the hash of the transaction. |
There was a problem hiding this comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements 📃 #824 (comment)
mina: MinaProtocol/mina#15137
o1js-bindings: o1-labs/o1js-bindings#245