-
Notifications
You must be signed in to change notification settings - Fork 82
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
feat: implement claimKghReward function #348
feat: implement claimKghReward function #348
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
7520f30
to
edd6653
Compare
if (isCompounding) { | ||
_delegate(validator, delegator, totalBoostedReward); | ||
} |
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.
It seems that this can be handled when calling _claimBoostedReward()
since totalBoostedReward
is returned.
If compounding is needed, caller can delegate the totalBoostedReward
to validator. wdyt?
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.
I didn't get what you mean. Could you elaborate it?
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.
Ok. I suggest to remove isCompounding
flag and just return totalBoostedReward
in this function.
Then, caller can choose to delegate the totalBoostedReward
or not.
At first, I thought the compounding delegation should be executed in this function, but it seems to be out of this function.
For example, in claimKghReward
, just transfer the totalBoostedReward
to the delegator. On the other hand, in delegateKgh
(would be handled in #349), make delegate with the totalBoostedReward
. Then we don't need the isCompounding
flang here.
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.
I agree it. Just return totalBoostedReward and handle auto compounding out of this function.
Additionally, I suggest to add internal view functions to calculate boosted reward and base reward of KGH delegator each, and use the two functions in getKghReward
and claimKghReward
. wdyt?
Then kghDelegator.rewardPerKghPaid = rewardPerKghStored;
this part should be handled in claimKghReward
and delegateKgh
.
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.
So you mean that the compounding process will be executed separately in delegateKgh
? Make sense, I'll remove it!
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.
Additionally, I suggest to add internal view functions to calculate boosted reward and base reward of KGH delegator each, and use the two functions in getKghReward and claimKghReward. wdyt?
Then kghDelegator.rewardPerKghPaid = rewardPerKghStored; this part should be handled in claimKghReward and delegateKgh.
I've also thought about this, but in terms of base reward, making an internal function is not desirable in terms of gas usage, since we have to check delegateAt
inside the for loop in claimKghReward
. Also for the boosted reward, _claimBoostedReward
is also used in delegateKgh
, which means an internal function to calculate the reward makes kghDelegator.rewardPerKghPaid = rewardPerKghStored;
line repeated inside delegateKgh
function, and I didn't see much differences in code readability imo. What do you think?
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.
OK, it's not that critical thing and I just suggested it to reduce code redundancy between the getKghReward
function and claimKghReward
function.
edd6653
to
86b9eaf
Compare
if (isCompounding) { | ||
_delegate(validator, delegator, totalBoostedReward); | ||
} |
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.
I agree it. Just return totalBoostedReward and handle auto compounding out of this function.
Additionally, I suggest to add internal view functions to calculate boosted reward and base reward of KGH delegator each, and use the two functions in getKghReward
and claimKghReward
. wdyt?
Then kghDelegator.rewardPerKghPaid = rewardPerKghStored;
this part should be handled in claimKghReward
and delegateKgh
.
a90762a
to
757bddf
Compare
* @param validator Address of the validator. | ||
* @param delegator Address of the delegator. | ||
* @param isCompounding Flag to auto compound the boosted reward. |
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.
* @param validator Address of the validator. | |
* @param delegator Address of the delegator. | |
* @param isCompounding Flag to auto compound the boosted reward. | |
* @param validator Address of the validator. | |
* @param delegator Address of the delegator. |
757bddf
to
b12cae3
Compare
* @param validator Address of the validator. | ||
* @param delegator Address of the delegator. |
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.
Can you remove the spaces?
* @param validator Address of the validator. | |
* @param delegator Address of the delegator. | |
* @param validator Address of the validator. | |
* @param delegator Address of the delegator. |
b12cae3
to
103171a
Compare
6e0ec89
into
feat/implement-validator-system-v2
Description
Implement
claimKghReward
function. Includes_claimBoostedReward
which hasisCompounding
flag.