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

[cli] Use simulation errors from API #5526

Merged
merged 1 commit into from
Dec 8, 2022
Merged

Conversation

gregnazario
Copy link
Contributor

@gregnazario gregnazario commented Nov 9, 2022

Description

The API errors for simulation are fixed, so we will use those now in the cli rather than fetching from the ABI

Test Plan

Tested manually as both a success and a failure:

$ ./target/debug/aptos account transfer --account 0x1337 --amount 999999999999999999
{
  "Error": "Simulation failed with status: Move abort in 0x1::coin: EINSUFFICIENT_BALANCE(0x10006): Not enough coins to complete transaction"
}

$ ./target/debug/aptos account transfer --account 0x1337 --amount 1                
Do you want to submit a transaction for a range of [184400 - 276600] Octas at a gas unit price of 100 Octas? [yes/no] >
y
{
  "Result": {
    "gas_unit_price": 100,
    "gas_used": 1878,
...

This change is Reviewable

@davidiw
Copy link
Contributor

davidiw commented Dec 3, 2022

This looks fine, let's get it tested and in... I'd be fine with just eye balling it...

API Errors for simulation are fixed, so we use these now.
Additionally, gas estimation and associated TODOs are cleaned
up.
@gregnazario gregnazario marked this pull request as ready for review December 6, 2022 23:13
@gregnazario gregnazario requested a review from banool as a code owner December 6, 2022 23:13
@gregnazario gregnazario requested a review from davidiw December 6, 2022 23:13
@gregnazario gregnazario enabled auto-merge (rebase) December 6, 2022 23:14
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

✅ Forge suite land_blocking success on eece5d273ef8732fd3eeb12150d6cb574cfb4f78

performance benchmark with full nodes : 6850 TPS, 5793 ms latency, 9300 ms p99 latency,no expired txns
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> eece5d273ef8732fd3eeb12150d6cb574cfb4f78

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> eece5d273ef8732fd3eeb12150d6cb574cfb4f78 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7466 TPS, 5214 ms latency, 6900 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: eece5d273ef8732fd3eeb12150d6cb574cfb4f78
compatibility::simple-validator-upgrade::single-validator-upgrade : 4416 TPS, 9531 ms latency, 12700 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: eece5d273ef8732fd3eeb12150d6cb574cfb4f78
compatibility::simple-validator-upgrade::half-validator-upgrade : 4419 TPS, 9230 ms latency, 12400 ms p99 latency,no expired txns
4. upgrading second batch to new version: eece5d273ef8732fd3eeb12150d6cb574cfb4f78
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6829 TPS, 5760 ms latency, 9200 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> eece5d273ef8732fd3eeb12150d6cb574cfb4f78 passed
Test Ok

Copy link
Contributor

@banool banool left a comment

Choose a reason for hiding this comment

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

This is a breaking change, what if someone relies on simulate_transaction? I suppose the Rust SDK probably isn't considered at 1.0.0 yet though, so not a big deal. Though if there is a way to make simulate_transaction call into the new function that'd be good.

@gregnazario
Copy link
Contributor Author

This is a breaking change, what if someone relies on simulate_transaction? I suppose the Rust SDK probably isn't considered at 1.0.0 yet though, so not a big deal. Though if there is a way to make simulate_transaction call into the new function that'd be good.

Deleted function is in the CLI, so unless someone's building on top of internal functions in the CLI, it should be fine.

@gregnazario gregnazario merged commit 2108b03 into main Dec 8, 2022
@gregnazario gregnazario deleted the cli-simulate-errors branch December 8, 2022 17:45
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.

4 participants