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

[TS SDK] Return txn data on success and txn failure reason on failure #9433

Merged
merged 2 commits into from
Aug 4, 2023

Conversation

0xmaayan
Copy link
Contributor

@0xmaayan 0xmaayan commented Aug 2, 2023

Description

The sdk provides a way to return the txn data when it succeed, added an example/test on how to use it.

For when a txn has failed, we throw an error with the txn hash and the failure reason message - Error class is a bit weird as it only outputs the "message" prop, unless we wrap it with try/catch and then we can get access to the other class properties, it feels like a big change for this use case and that should be good for now.

Test Plan

@@ -672,7 +672,7 @@ export class AptosClient {
}
if (!(lastTxn as any)?.success) {
throw new FailedTransactionError(
`Transaction ${txnHash} committed to the blockchain but execution failed`,
`Transaction ${txnHash} failed with an error: ${(lastTxn as any).vm_status}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay :D Is there any way we can put the vm_status in a field in the FailedTransactionError class? I actually don't know if this makes sense, but it would make it easier to use instead of parsing it. Is it generally not a good idea to add fields to existing classes if the sdk exposes them with export, or would it not really matter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have the full txn object part of the FailedTransactionError class. but as mentioned in the PR comment, Error class is a bit weird as it only outputs the "message" prop, unless we wrap it with try/catch and then we can get access to the other class properties, it feels like a big change for this use case and that should be good for now.

We can always just return the full transaction object, but then we lost the "message" part, i.e you won't see Transaction ${txnHash} failed with an error: ${(lastTxn as any).vm_status} but only a txn object and then you would need to look for the success prop to see if it succeed or failed - basically same response for a success txn

Copy link
Contributor

Choose a reason for hiding this comment

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

oh duh, I forgot the txn object is in the error! Yeah this is great, I think it makes most sense this way then

@0xmaayan 0xmaayan enabled auto-merge (squash) August 4, 2023 01:58
@0xmaayan 0xmaayan force-pushed the return_txn_response branch from 5e3aa1d to 7dffa25 Compare August 4, 2023 02:03
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2023

✅ Forge suite compat success on aptos-node-v1.5.1 ==> 7dffa25261dd8a18081ad3e7431a13dd15d9f845

Compatibility test results for aptos-node-v1.5.1 ==> 7dffa25261dd8a18081ad3e7431a13dd15d9f845 (PR)
1. Check liveness of validators at old version: aptos-node-v1.5.1
compatibility::simple-validator-upgrade::liveness-check : committed: 4033 txn/s, latency: 7306 ms, (p50: 7300 ms, p90: 10500 ms, p99: 12400 ms), latency samples: 165380
2. Upgrading first Validator to new version: 7dffa25261dd8a18081ad3e7431a13dd15d9f845
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1777 txn/s, latency: 15625 ms, (p50: 19000 ms, p90: 21700 ms, p99: 22300 ms), latency samples: 92420
3. Upgrading rest of first batch to new version: 7dffa25261dd8a18081ad3e7431a13dd15d9f845
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1770 txn/s, latency: 15907 ms, (p50: 19400 ms, p90: 22000 ms, p99: 22700 ms), latency samples: 92060
4. upgrading second batch to new version: 7dffa25261dd8a18081ad3e7431a13dd15d9f845
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3371 txn/s, latency: 9347 ms, (p50: 10800 ms, p90: 12400 ms, p99: 12900 ms), latency samples: 134840
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 7dffa25261dd8a18081ad3e7431a13dd15d9f845 passed
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2023

✅ Forge suite realistic_env_max_load success on 7dffa25261dd8a18081ad3e7431a13dd15d9f845

two traffics test: inner traffic : committed: 6130 txn/s, latency: 6390 ms, (p50: 6200 ms, p90: 7800 ms, p99: 12300 ms), latency samples: 2654440
two traffics test : committed: 100 txn/s, latency: 2888 ms, (p50: 2800 ms, p90: 3500 ms, p99: 6700 ms), latency samples: 1840
Max round gap was 1 [limit 4] at version 994192. Max no progress secs was 3.756553 [limit 10] at version 994192.
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.5.1 ==> 7dffa25261dd8a18081ad3e7431a13dd15d9f845

Compatibility test results for aptos-node-v1.5.1 ==> 7dffa25261dd8a18081ad3e7431a13dd15d9f845 (PR)
Upgrade the nodes to version: 7dffa25261dd8a18081ad3e7431a13dd15d9f845
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 4443 txn/s, latency: 7369 ms, (p50: 7500 ms, p90: 10100 ms, p99: 12300 ms), latency samples: 164400
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 7dffa25261dd8a18081ad3e7431a13dd15d9f845 passed
Test Ok

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.

3 participants