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

Fix: tips scaling in Summit #1463

Merged
merged 3 commits into from
Oct 23, 2023
Merged

Fix: tips scaling in Summit #1463

merged 3 commits into from
Oct 23, 2023

Conversation

ChiTimesChi
Copy link
Collaborator

@ChiTimesChi ChiTimesChi commented Oct 19, 2023

Description
Fixes a bug where Summit was storing actor tip values scaled down by 2**32 (as they appear in the message payload). As a result, the actors (bonded agents and unbonded executors) were previously gaining a tiny fraction of the message tips.

Summary by CodeRabbit

  • New Feature: Introduced a new system to track and manage tips earned by actors in the Summit contract. This feature enhances the transparency and accuracy of tip distribution.
  • Documentation: Updated the description of the TipAwarded event in the SummitEvents contract to reflect the new tip calculation method.
  • Test: Enhanced the SummitTipsTest contract with a new function to simulate the awarding of tips and improved existing functions for better accuracy in testing the earned tips of an actor.

These changes aim to improve the overall user experience by providing a more precise and transparent system for managing and tracking tips.

@ChiTimesChi ChiTimesChi requested a review from trajan0x as a code owner October 19, 2023 16:29
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 19, 2023

Walkthrough

The changes primarily focus on the implementation of a new tipping system in the Summit contract. The system scales up the tip value for better precision and modifies the associated events and tests accordingly.

Changes

File Summary
.../contracts/Summit.sol Introduced a new struct ActorTips to store tips data. The _awardActorTip function now scales up the tip value using TIPS_GRANULARITY. The TipAwarded event emits the scaled-up tip value.
.../contracts/events/SummitEvents.sol Updated the TipAwarded event documentation to reflect the change in tip denomination.
.../test/suite/SummitTips.t.sol Added a new function expectAwardedTipsEvents to simulate tip awarding. Refactored checkActorTips to checkEarnedActorTips to check the scaled-up earned tips.

🐇
"In the land of code, where logic is the road,
A change was made, a new feature to load.
Tips are now grand, scaled up by demand,
Tests and events, all perfectly planned. 🎉"


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.
  • Additionally, enter @coderabbitai pause to halt the bot's ongoing reviews as any issue or review comment. To
    resume reviews, enter @coderabbitai resume as any issue or review comment.

@github-actions github-actions bot added M-contracts Module: Contracts 2-reviewers C-Protocol-Critical PRs that modify protocol-critical code. size/s labels Oct 19, 2023
@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (4416f0a) 50.69210% compared to head (65e0e00) 50.69407%.
Report is 16 commits behind head on master.

Additional details and impacted files
@@                 Coverage Diff                 @@
##              master       #1463         +/-   ##
===================================================
+ Coverage   50.69210%   50.69407%   +0.00197%     
===================================================
  Files            356         356                 
  Lines          24274       24277          +3     
  Branches         267         268          +1     
===================================================
+ Hits           12305       12307          +2     
  Misses         10775       10775                 
- Partials        1194        1195          +1     
Flag Coverage Δ
packages 90.62003% <ø> (ø)
solidity 93.01948% <100.00000%> (-0.06433%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
packages/contracts-core/contracts/Destination.sol 87.50000% <100.00000%> (ø)
packages/contracts-core/contracts/GasOracle.sol 100.00000% <100.00000%> (ø)
packages/contracts-core/contracts/Origin.sol 95.45455% <100.00000%> (+0.21644%) ⬆️
packages/contracts-core/contracts/Summit.sol 96.62921% <100.00000%> (+0.03830%) ⬆️
...ages/contracts-core/contracts/hubs/SnapshotHub.sol 83.19328% <100.00000%> (ø)
.../contracts-core/contracts/inbox/StatementInbox.sol 89.74359% <100.00000%> (ø)
...ontracts-core/contracts/manager/BondingManager.sol 86.58537% <100.00000%> (ø)
.../contracts-core/contracts/manager/LightManager.sol 96.66667% <100.00000%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between dcda614 and 65e0e00.
Files selected for processing (3)
  • packages/contracts-core/contracts/Summit.sol (3 hunks)
  • packages/contracts-core/contracts/events/SummitEvents.sol (1 hunks)
  • packages/contracts-core/test/suite/SummitTips.t.sol (5 hunks)
Files skipped from review due to trivial changes (1)
  • packages/contracts-core/contracts/events/SummitEvents.sol
Additional comments (Suppressed): 9
packages/contracts-core/contracts/Summit.sol (3)
  • 4-10: The import statement for TIPS_GRANULARITY from ./libs/Constants.sol has been added. Ensure that this constant is defined in the Constants.sol file and that its value is appropriate for the shift operation in line 335.

  • 61-69: The ActorTips struct now includes comments for its fields earned and claimed. This is a good practice as it improves code readability and understanding.

  • 329-338: The _awardActorTip function has been modified to scale up the tip value before adding it to the earned field of actorTips. This is done to store the full tip value in actorTips as opposed to the scaled-down value. The TipAwarded event now emits the scaled-up tip value. Ensure that this change is reflected in all parts of the codebase where this event is handled.

-        actorTips[actor][origin].earned += tip;
-        emit TipAwarded(actor, origin, tip);
+        uint128 tipAwarded = uint128(tip) << uint128(TIPS_GRANULARITY);
+        actorTips[actor][origin].earned += tipAwarded;
+        emit TipAwarded(actor, origin, tipAwarded);
packages/contracts-core/test/suite/SummitTips.t.sol (6)
  • 238-245: The new function expectAwardedTipsEvents is called before distributeTips and checkAwardedTips. This function seems to simulate the awarding of tips and is expected to generate events that are then checked in the checkAwardedTips function. Ensure that this function is working as expected and that it does not interfere with the actual distribution of tips.

  • 254-261: Similar to the previous comment, the expectAwardedTipsEvents function is called before distributeTips and checkAwardedTips. Ensure that this function is working as expected and that it does not interfere with the actual distribution of tips.

  • 278-284: Again, the expectAwardedTipsEvents function is called before distributeTips and checkAwardedTips. Ensure that this function is working as expected and that it does not interfere with the actual distribution of tips.

  • 377-408: The function checkActorTips has been renamed to checkEarnedActorTips and refactored to use a scaled-down version of earned tips, which is then scaled up by 2**32 when checking the earned tips. This change is in line with the PR summary and seems to be a necessary adjustment to ensure that actors receive the correct amount of tips. However, ensure that this scaling operation does not introduce any rounding errors or inaccuracies in the calculation of tips.

  • 412-425: The function checkActorTips has been renamed to checkEarnedActorTips and refactored to use a scaled-down version of earned tips, which is then scaled up by 2**32 when checking the earned tips. This change is in line with the PR summary and seems to be a necessary adjustment to ensure that actors receive the correct amount of tips. However, ensure that this scaling operation does not introduce any rounding errors or inaccuracies in the calculation of tips.

  • 426-504: The new functions expectAwardedTipsEvents, expectAwardedTipsEventsFirstSubmit, expectAwardedTipsEventsReceiptTips, expectAwardedTipsEventsFinalSubmit, expectAwardedTipsEvent, and checkEarnedActorTips have been added. These functions seem to simulate the awarding of tips and the generation of events, and then check the earned tips. Ensure that these functions are working as expected and that they do not interfere with the actual distribution of tips. Also, ensure that the scaling operation in checkEarnedActorTips does not introduce any rounding errors or inaccuracies in the calculation of tips.

@ChiTimesChi
Copy link
Collaborator Author

@ChiTimesChi ChiTimesChi merged commit 04ba913 into master Oct 23, 2023
51 checks passed
@ChiTimesChi ChiTimesChi deleted the fix/msg/tips-scaling branch October 23, 2023 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2-reviewers C-Protocol-Critical PRs that modify protocol-critical code. M-contracts Module: Contracts needs-go-generate-agents size/s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants