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

BugFix: ATC error message improvement #463

Merged
merged 4 commits into from
Apr 6, 2023
Merged

Conversation

barnjamin
Copy link
Contributor

@barnjamin barnjamin commented Apr 6, 2023

closes #462

@barnjamin barnjamin changed the title fix: ATC error message improvement BugFix: ATC error message improvement Apr 6, 2023
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Looks good but why change steps.py?

@@ -32,7 +32,7 @@ def parse_string(text):
algod_port = 60000
kmd_port = 60001

DEV_ACCOUNT_INITIAL_MICROALGOS: int = 10_000_000
DEV_ACCOUNT_INITIAL_MICROALGOS: int = 100_000_000
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this here too? Seems completely unrelated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests have been v flakey with issues related to overspend errors. This change was an experiment to see if it fixed the overspend errors.

The tests passed on the first attempt once I included it, but I cant be sure this is the fix

Copy link
Contributor

Choose a reason for hiding this comment

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

I outlined the rationale for this one liner here as well: #459

It resolves current flaky tests because rekeying will change how transient accounts are funded in a non-deterministic way

Copy link
Contributor

Choose a reason for hiding this comment

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

We already do this in the JS SDK but swept it under the rug in another PR: https://github.com/algorand/js-algorand-sdk/blob/develop/tests/cucumber/steps/steps.js#L122

Copy link
Contributor

Choose a reason for hiding this comment

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

The rekey changes the ordering of the kmd wallets in py-algorand-sdk (I believe)
i think the change should be to provide non-deterministic access to the main network accounts instead

Copy link
Contributor

@algochoi algochoi left a comment

Choose a reason for hiding this comment

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

This looks like a good UX improvement, and I also think the one liner change in steps.py is nice to immediately fix any flaky tests

@barnjamin barnjamin merged commit 44db7b2 into develop Apr 6, 2023
@barnjamin barnjamin deleted the atc-err-improvement branch April 6, 2023 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Difficult to debug error message for ATC
4 participants