-
Notifications
You must be signed in to change notification settings - Fork 41
test: add update_rewards toggle tokens flow test #57
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
base: main
Are you sure you want to change the base?
test: add update_rewards toggle tokens flow test #57
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
==========================================
+ Coverage 95.74% 95.78% +0.03%
==========================================
Files 42 42
Lines 10041 10133 +92
==========================================
+ Hits 9614 9706 +92
Misses 427 427 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
17f1baf to
a3d9239
Compare
1765bb9 to
3573ad2
Compare
noa-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @arad-starkware)
src/flow_test/test.cairo line 3039 at r3 (raw file):
/// Advance K epochs /// Enable token A, disable token B /// update_rewards
I think we need this test for both v2 rewards and consensus rewards, please update the test
src/flow_test/flows.cairo line 7070 at r3 (raw file):
/// Add tokens A and B /// Enable token B /// Advance epoch
rebase issues?
484a333 to
215d37b
Compare
arad-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @noa-starkware)
src/flow_test/test.cairo line 3039 at r3 (raw file):
Previously, noa-starkware wrote…
I think we need this test for both v2 rewards and consensus rewards, please update the test
I'm not sure about the flow you're describing, do you mean specifically v2 rewards and consensus in the same test? we have MultipleTokensDelegationFlow for v2 rewards otherwise
src/flow_test/flows.cairo line 7070 at r3 (raw file):
Previously, noa-starkware wrote…
rebase issues?
yes
550520d to
a9c72c0
Compare
894e551 to
38e57fd
Compare
a9c72c0 to
1d21b45
Compare
65039fa to
d3ed532
Compare
1d21b45 to
943c3b0
Compare
noa-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @arad-starkware)
src/flow_test/test.cairo line 3094 at r6 (raw file):
// update_rewards - test rewards only for token B system.advance_block_into_attestation_window(:staker); system.attest(:staker);
test also staker rewards please
src/flow_test/test.cairo line 3111 at r6 (raw file):
// update_rewards - test rewards only for token B system.advance_block_into_attestation_window(:staker); system.attest(:staker);
test also staker rewards please
src/flow_test/test.cairo line 3120 at r6 (raw file):
// update_rewards - test rewards only for token A system.advance_block_into_attestation_window(:staker); system.attest(:staker);
test also staker rewards please
src/flow_test/test.cairo line 3128 at r6 (raw file):
// Start consensus rewards system.start_consensus_rewards();
// Enable token B, disable token A
src/flow_test/test.cairo line 3148 at r6 (raw file):
// update_rewards - test rewards only for token A system.update_rewards(:staker, disable_rewards: false);
test also staker rewards please
src/flow_test/test.cairo line 3156 at r6 (raw file):
// update_rewards - test rewards only for token A system.update_rewards(:staker, disable_rewards: false);
test also staker rewards please
943c3b0 to
25ecf8f
Compare
| let delegator_b_rewards = system.delegator_claim_rewards(delegator: delegator_b, pool: pool_b); | ||
| assert!(staker_rewards == expected_staker_rewards + commission_rewards); | ||
| assert!(delegator_a_rewards.is_zero()); | ||
| assert!(delegator_b_rewards == expected_delegator_rewards); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Wrong expected rewards used for token B validation
The test calculates expected_delegator_rewards using token A's parameters (delegation_amount_a, token_a_decimals, token_a.contract_address()) at lines 3163-3174, but then uses this value to validate token B's rewards at line 3206. Since token A has 8 decimals with min_for_rewards of 10³ and token B has 18 decimals with min_for_rewards of 10¹³, the expected rewards will be incorrect. The test needs to recalculate expected rewards using token B's configuration before the final assertion block.
Additional Locations (1)
25ecf8f to
ea98f96
Compare
arad-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 7 unresolved discussions (waiting on @noa-starkware)
src/flow_test/test.cairo line 3094 at r6 (raw file):
Previously, noa-starkware wrote…
test also staker rewards please
Done.
src/flow_test/test.cairo line 3111 at r6 (raw file):
Previously, noa-starkware wrote…
test also staker rewards please
Done.
src/flow_test/test.cairo line 3120 at r6 (raw file):
Previously, noa-starkware wrote…
test also staker rewards please
Done.
src/flow_test/test.cairo line 3128 at r6 (raw file):
Previously, noa-starkware wrote…
// Enable token B, disable token A
Done
src/flow_test/test.cairo line 3148 at r6 (raw file):
Previously, noa-starkware wrote…
test also staker rewards please
Done.
src/flow_test/test.cairo line 3156 at r6 (raw file):
Previously, noa-starkware wrote…
test also staker rewards please
Done.
ea98f96 to
96cb2c9
Compare
d3ed532 to
071a55d
Compare
625acb6 to
d3512f2
Compare
6ad074b to
5f4a056
Compare
d3512f2 to
5270e9e
Compare
5f4a056 to
3ee5f59
Compare
5270e9e to
8a0dadc
Compare
8a0dadc to
bbf04ed
Compare
| let staker_rewards = system.staker_claim_rewards(:staker); | ||
| let delegator_a_rewards = system.delegator_claim_rewards(delegator: delegator_a, pool: pool_a); | ||
| let delegator_b_rewards = system.delegator_claim_rewards(delegator: delegator_b, pool: pool_b); | ||
| assert!(staker_rewards == expected_staker_rewards + commission_rewards); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Staker commission uses wrong token's parameters for assertions
At line 3142, when token A becomes active (comment says "test rewards only for token A"), the staker_rewards assertion uses commission_rewards that was calculated for token B (lines 3109-3115). Similarly at line 3203, when token B becomes active, the assertion uses commission_rewards calculated for token A (lines 3162-3175). The staker commission should reflect the currently active token's pool, but the test reuses commission values calculated for the previously active token. This passes only due to the coincidental numerical equivalence after normalization.
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment why its ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
bbf04ed to
eb7dc58
Compare
noa-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@noa-starkware reviewed 1 of 2 files at r11.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @arad-starkware)
src/flow_test/test.cairo line 3244 at r11 (raw file):
system.staking.disable_token(token_address: token_a.contract_address()); // Calculate rewards for consensus rewards
- only for one token enabled
src/flow_test/test.cairo line 3269 at r11 (raw file):
// update_rewards - test rewards only for token A system.update_rewards(:staker, disable_rewards: false);
update_rewards twice before advance epoch?
| let staker_rewards = system.staker_claim_rewards(:staker); | ||
| let delegator_a_rewards = system.delegator_claim_rewards(delegator: delegator_a, pool: pool_a); | ||
| let delegator_b_rewards = system.delegator_claim_rewards(delegator: delegator_b, pool: pool_b); | ||
| assert!(staker_rewards == expected_staker_rewards + commission_rewards); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment why its ok
eb7dc58 to
41be9ca
Compare
arad-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 3 files reviewed, all discussions resolved
src/flow_test/test.cairo line 3244 at r11 (raw file):
Previously, noa-starkware wrote…
- only for one token enabled
Done
src/flow_test/test.cairo line 3269 at r11 (raw file):
Previously, noa-starkware wrote…
update_rewards twice before advance epoch?
Done
| let staker_rewards = system.staker_claim_rewards(:staker); | ||
| let delegator_a_rewards = system.delegator_claim_rewards(delegator: delegator_a, pool: pool_a); | ||
| let delegator_b_rewards = system.delegator_claim_rewards(delegator: delegator_b, pool: pool_b); | ||
| assert!(staker_rewards == expected_staker_rewards + commission_rewards); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
41be9ca to
ddcbf57
Compare
ddcbf57 to
0b18f21
Compare

This change is
Note
Adds a comprehensive flow test validating staker/delegator rewards when toggling BTC tokens across attestation and consensus epochs, plus trims related flow ideas.
update_rewards_token_enable_disable_flow_testinsrc/flow_test/test.cairo:update_rewardscalls in the same epoch and normalized balance calculations.k=1 -> k=2 tokeninsrc/flow_test/flow_ideas.md.Written by Cursor Bugbot for commit 0b18f21. This will update automatically on new commits. Configure here.