test: add toggle tokens before and after upgrade test#51
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
692a1de to
95239a7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
==========================================
+ Coverage 95.43% 95.60% +0.16%
==========================================
Files 45 45
Lines 9446 9783 +337
==========================================
+ Hits 9015 9353 +338
+ Misses 431 430 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
noa-starkware
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @NirLevi-starkware)
src/flow_test/flows.cairo line 7067 at r1 (raw file):
} /// Flow:
I think we should also test disable/enable in the same epoch of upgrade. then test same and next epoch after upgrade
src/flow_test/flows.cairo line 7070 at r1 (raw file):
/// Token A enabled, Token B disabled /// Test tokens /// Upgrade to V3
test here
src/flow_test/flows.cairo line 7092 at r1 (raw file):
system.staking.enable_token(token_address: token_a.contract_address()); let result = system.staking.safe_disable_token(token_address: token_b.contract_address()); assert_panic_with_error(result, StakingError::TOKEN_ALREADY_DISABLED.describe());
I think these lines are unnecessary
Code quote:
let result = system.staking.safe_disable_token(token_address: token_b.contract_address());
assert_panic_with_error(result, StakingError::TOKEN_ALREADY_DISABLED.describe());src/flow_test/flows.cairo line 7100 at r1 (raw file):
] .span(); assert!(tokens == expected_tokens);
same, what are you trying to test here?
Code quote:
let tokens = system.staking.dispatcher().get_tokens();
let expected_tokens = array![
(STRK_TOKEN_ADDRESS, true), (system.btc_token.contract_address(), true),
(token_a.contract_address(), true), (token_b.contract_address(), false),
]
.span();
assert!(tokens == expected_tokens);src/flow_test/flows.cairo line 7115 at r1 (raw file):
] .span(); assert!(tokens == expected_tokens);
add test with rewards? WDYT?
src/flow_test/flows.cairo line 7135 at r1 (raw file):
] .span(); assert!(tokens == expected_tokens);
Also here?
95239a7 to
c36a370
Compare
29b6f24 to
4939c4b
Compare
arad-starkware
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @NirLevi-starkware and @noa-starkware)
src/flow_test/flows.cairo line 7067 at r1 (raw file):
Previously, noa-starkware wrote…
I think we should also test disable/enable in the same epoch of upgrade. then test same and next epoch after upgrade
Done.
src/flow_test/flows.cairo line 7070 at r1 (raw file):
Previously, noa-starkware wrote…
test here
Done.
src/flow_test/flows.cairo line 7100 at r1 (raw file):
Previously, noa-starkware wrote…
same, what are you trying to test here?
nvm
src/flow_test/flows.cairo line 7115 at r1 (raw file):
Previously, noa-starkware wrote…
add test with rewards? WDYT?
There's a different flow (idea) that tests rewards, or do you mean specifically after upgrade?
src/flow_test/flows.cairo line 7135 at r1 (raw file):
Previously, noa-starkware wrote…
Also here?
see above
noa-starkware
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @arad-starkware and @NirLevi-starkware)
src/flow_test/flows.cairo line 7115 at r1 (raw file):
Previously, arad-starkware wrote…
There's a different flow (idea) that tests rewards, or do you mean specifically after upgrade?
yes, lets add a test with one token disabled + one token enabled before upgrade, then test after upgrade view+rewards. Please add to flow ideas now and open a new pr for that later.
src/flow_test/flows.cairo line 7128 at r2 (raw file):
system.staking.enable_token(token_address: token_b.contract_address()); let tokens = system.staking.dispatcher().get_tokens(); assert!(tokens == expected_tokens);
Suggestion:
// Test same epoch.
let tokens = system.staking.dispatcher().get_tokens();
assert!(tokens == expected_tokens);c36a370 to
affafed
Compare
af890b8 to
f1a3f80
Compare
4a7e18c to
bca5ae2
Compare
f1a3f80 to
149e559
Compare
1c79687 to
d08a2c7
Compare
arad-starkware
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @NirLevi-starkware and @noa-starkware)
src/flow_test/flows.cairo line 7115 at r1 (raw file):
Previously, noa-starkware wrote…
yes, lets add a test with one token disabled + one token enabled before upgrade, then test after upgrade view+rewards. Please add to flow ideas now and open a new pr for that later.
Done.
src/flow_test/flows.cairo line 7128 at r2 (raw file):
system.staking.enable_token(token_address: token_b.contract_address()); let tokens = system.staking.dispatcher().get_tokens(); assert!(tokens == expected_tokens);
Done
noa-starkware
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @NirLevi-starkware)
src/flow_test/flow_ideas.md line 103 at r3 (raw file):
- same as above with disable (can be implemented together as one test) - token A enabled, next epoch token B enabled, next epoch token A disabled, next epoch token B disabled - enable token A and disable token B, upgrade (in the same epoch), views and rewards the next few epochs
Why? Thats what we implemented now. isnt it?
Code quote:
(in the same epoch)d08a2c7 to
c7d5511
Compare
arad-starkware
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @NirLevi-starkware and @noa-starkware)
src/flow_test/flow_ideas.md line 103 at r3 (raw file):
Previously, noa-starkware wrote…
Why? Thats what we implemented now. isnt it?
I thought you said you wanted to test with rewards, then I don't understand what the test it... just that after the upgrade the tokens are still in the same state?
Took me writing this comment to understand the connection between this test and what we talked about...
c7d5511 to
dcb0558
Compare
149e559 to
894eed0
Compare
894eed0 to
c44bef8
Compare
dcb0558 to
6c99136
Compare
6c99136 to
5dbaf64
Compare
c44bef8 to
411f3b9
Compare
5dbaf64 to
d8f66ad
Compare
noa-starkware
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @NirLevi-starkware)
Merge activity
|

This change is
Note
Adds a flow test that toggles BTC tokens around an upgrade and asserts token activation state across epochs, with a mainnet-fork test and docs update.
ToggleTokensBeforeAfterUpgradeFlowinsrc/flow_test/flows.cairoto add two BTC tokens, toggle enable/disable around an upgrade, and verifydispatcher().get_tokens()across epochs.toggle_tokens_before_after_upgrade_flow_testinsrc/flow_test/fork_test.cairo.src/flow_test/flow_ideas.mdwith the token A/B enable/disable + upgrade scenario.flows.cairo(addTokenandSTRK_TOKEN_ADDRESS).Written by Cursor Bugbot for commit d8f66ad. This will update automatically on new commits. Configure here.