Skip to content

Conversation

@aliXsed
Copy link
Collaborator

@aliXsed aliXsed commented Jun 20, 2024

In this implementation I have defined the struct BatchReward as:

    struct BatchReward {
        address[] recipients;
        uint256[] amounts;
        uint256 sequence;
    }

assuming recipients[i] corresponds to amounts[i]. This means we expect recipients.length == amounts.length. That's why mintBatchReward needed to check this assumption. The reason I have gone for this design was to avoid using nested types which could make digestReward more complicated and costly. digestReward is needed for checking the signature.

@aliXsed aliXsed requested a review from ETeissonniere June 20, 2024 01:18
@aliXsed aliXsed marked this pull request as ready for review June 20, 2024 01:18
@aliXsed aliXsed requested a review from trsmarc June 20, 2024 02:27
Copy link
Member

@ETeissonniere ETeissonniere left a comment

Choose a reason for hiding this comment

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

No real comments, just whitespacing (we don't need one space between every line :D) and one open question on a double for loop which might be optimizable

Comment on lines 217 to 226
uint256 batchSum = _batchSum(batch);

_checkedUpdateClaimed(batchSum);

// Safe to increment the sequence after checking this is the expected number (no overflow for the age of universe even with 1000 reward claims per second)
batchSequence = batch.sequence + 1;

for (uint256 i = 0; i < batch.recipients.length; i++) {
nodl.mint(batch.recipients[i], batch.amounts[i]);
}
Copy link
Member

Choose a reason for hiding this comment

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

We can avoid the double for loop here (other loop in _batchSum) by rewriting the for iteration and add one last revert statement afterwards

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tested the potential gas saving in your suggestion and found it either absolute zero or very negligible (less than 0.1%). While checking first and minting later has the benefit of early failure (gas saving there) and follows the best practice of updating state first before calling into any other contract.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, let's keep it as is

Alex Sedighi and others added 2 commits June 21, 2024 13:18
Co-authored-by: Eliott Teissonniere <10683430+ETeissonniere@users.noreply.github.com>
@aliXsed aliXsed merged commit 559361e into main Jun 21, 2024
@aliXsed aliXsed deleted the aliX/batch-reward2 branch June 21, 2024 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants