Added integration test for auto-compounding. #4491
Conversation
github-actions
bot
added
the
backport-libs-0.48
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4491 +/- ##
==========================================
- Coverage 74.59% 0 -74.60%
==========================================
Files 339 0 -339
Lines 111402 0 -111402
==========================================
- Hits 83096 0 -83096
+ Misses 28306 0 -28306 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
murisi
force-pushed
the
murisi/test-auto-compounding
branch
from
bb614e8 to
649b176
Compare
crates/tests/src/integration/masp.rs
Outdated
| assert!(captured.result.is_ok()); | ||
| assert!( | ||
| captured.contains( | ||
| "Estimated native token rewards for the next MASP epoch: 0" |
There was a problem hiding this comment.
Since the original BTCs are still in the pool, shouldn't we see the extra rewards coming from the new epoch?
There was a problem hiding this comment.
Since we are manually compounding for AB, I think 0 is correct(?). But for AA, which is tested above, I think you have a point.
There was a problem hiding this comment.
It could also be due to this #4285 (comment)
There was a problem hiding this comment.
Since the original BTCs are still in the pool, shouldn't we see the extra rewards coming from the new epoch?
Thanks for bringing this up. I've just checked, and there are actually rewards. The call to captured.contains here is misleading because the string "Estimated ... next MASP epoch: 0" is technically contained by strings of the form "Estimated ... next MASP epoch: 0.xyz...".
There was a problem hiding this comment.
Since we are manually compounding for AB, I think 0 is correct(?). But for AA, which is tested above, I think you have a point.
Once the substring issue above is fixed, it seems that the estimate is not actually zero for AA or for AB. But is this what we expect? For AA (automatic compounding), I think this is what we all expected. For AB (manual compounding) this is what I would expect given my superficial understanding of the estimates algorithm. It takes the manually shielded balance after it is exchanged to the current epoch, and essentially reapplies/duplicates the conversions from the previous epoch to the current (after relabelling them to make it look like they convert from the current epoch to the next one in the future). @batconjurer Please let me know if we should still be concerned about the expected estimates given the latest push? Thanks!
There was a problem hiding this comment.
Yes, that's how it works, so there should be rewards for both cases
crates/tests/src/integration/masp.rs
Outdated
| assert!(captured.result.is_ok()); | ||
| assert!( | ||
| captured.contains( | ||
| "Estimated native token rewards for the next MASP epoch: 0" |
There was a problem hiding this comment.
Since we are manually compounding for AB, I think 0 is correct(?). But for AA, which is tested above, I think you have a point.
murisi
force-pushed
the
murisi/test-auto-compounding
branch
from
0cfcdae to
62a0614
Compare
|
Should this still be merged and worked on if #4285 is closed? |
murisi
force-pushed
the
murisi/test-auto-compounding
branch
from
5bcb7b4 to
bbb4a17
Compare
murisi
force-pushed
the
murisi/test-auto-compounding
branch
from
bbb4a17 to
77d5df2
Compare
I have attempted to address all the review comments, and am awaiting feedback. So the work here might already be complete.
I think there is value on merging this PR. Probably this PR does three things: 1) it verifies the invariant that manual compounding is equivalent to automatic compounding is satisfied, 2) it documents the existence of such an invariant on the shielded reward system (on the level of the
This PR is only applicable when #4285 is closed. This is because this PR checks that automatic compounding is equivalent to manual compounding while #4285 which removes automatic compounding. Concretely, if #4285 and #4491 were to be combined, the integration test in this PR would fail due to the unmet expectation that shielded balances automatically increase on each epoch. |
murisi
added
MASP
testing
merge
|
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks:
You may have to fix your CI before adding the pull request to the queue again. If you want to requeue this pull request, you can post a |
|
Hey @murisi, your pull request has been dequeued due to the following reason: CHECKS_FAILED. |
|
@Mergifyio requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
Describe your changes
Added an integration test to check that the automatic compounding of a shielded account balance is the same as manually compounding a shielded account balance. Here, manual compounding refers to a shielded pool user immediately unshielding shielded rewards they receive (thereby applying/realizing rewards at that point in time) and then reshielding them (with the timestamp of the current MASP epoch). Though this means that different allowed conversions (from the protocol) are now applied onto the automatically and manually compounded accounts, the results of shielded balance queries are expected to be the approximately the same for all MASP epochs.
See the results of compounding a principal amount of 0.1 BTC below:
Note that auto-compounding gives slightly higher balances than manual compounding. This is probably because NAM conversions to the latest epoch have ever higher thresholds which means that the dust that doesn't get converted will also get higher. Also note that the MASP balance is slightly higher than the claimable amount, which is the sum of the shielded balances held by all the shielded key holders. This is due to rounding errors in the deflation process as explained in #4372 . The growth of the gap between what is claimable and the MASP balance will depend on the inflation rate of NAM, and the relative precisions of NAM and the token being rewarded.
Checklist before merging
breaking::labelsnamada-docsreponamada-indexerornamada-masp-indexer, a corresponding PR is opened in that repo