Skip to content

feat: claim to a third-party address #152

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

Merged
merged 8 commits into from
May 13, 2025
Merged

Conversation

smol-ninja
Copy link
Member

@smol-ninja smol-ninja commented May 1, 2025

Changelog


  • The following function has been added:
function claimTo(uint256 index, address to, uint128 amount, bytes32[] calldata merkleProof) external payable;

Important

With claimTo, msg.sender, if eligible, can receive airdrop on to address. If msg.sender is not eligible, it should revert. msg.sender should not be able to claim on other's behalf. It applies to both direct transfers as well as stream.

Important

With claim, anyone can claim on anyone's behalf. But the token should always be sent to the eligible user. It applies to both direct transfers as well as stream.

  • The Claim event has been changed to become re-usable across both claim and claimTo (cc might be important for @sablier-labs/frontend-core):
event Claim(uint256 index, address indexed recipient, uint128 amount, address to);
event Claim(uint256 index, address indexed recipient, uint128 amount, address to);
event Claim(uint256 index, address indexed recipient, uint128 amount, uint256 indexed streamId, address to);
event Claim(uint256 index, address indexed recipient, uint128 claimAmount, uint128 forgoneAmount, address to);
  • Added a _postProcessClaim function to be reused across claim and claimTo.
  • Added fuzz tests for claimTo.
  • I have created a separate shared test for claimTo. Even though it shares logic with claim, a separate integration for claimTo is important for extra security.
  • Skipped fork tests to reduce load on RPCs since the logic is same across the two claim functions. Also, changing fork tests would require refactoring it similar to fuzz tests multi airdrop function. Also, both functions are thoroughly tests across integration and concrete tests.

@smol-ninja smol-ninja force-pushed the feat/claim-recipient branch from 50e8a60 to 0db4fdf Compare May 2, 2025 10:48
@smol-ninja smol-ninja force-pushed the feat/claim-recipient branch from 0db4fdf to 9439ebb Compare May 5, 2025 10:42
Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

Great work — I’ve left my feedback below:

I have created a separate shared test for claimTo. Even though it shares logic with claim, a separate integration for claimTo is important for extra security.

I agree with the idea of using the same approach of having a shared file.
However, similar to the createWithTimestamps vs createWithDurations tests, I believe we can just test one the branches below, since currently is testing the same logic twice:

├── when to address zero
│  └── it should revert
└── when to address not zero
   ├── when caller not eligible
   │  └── it should revert
   └── when caller eligible
      ├── it should mark the index as Claimed
      ├── it should transfer the ETH to the merkle lockup
      └── it should emit {Claim} event

@smol-ninja
Copy link
Member Author

Thank you very much @andreivladbrg for the review and the feedback.

I believe we can just test one the branches below, since currently is testing the same logic twice:

Which two branches are you talking about?

claimTo.tree and claim.tree do not have the same starting branches.

They might share the middle branches but that would be a bad DX. Also, in one test, we are referring to caller whereas in other, we are referring to recipient for the clarity.

@andreivladbrg
Copy link
Member

andreivladbrg commented May 12, 2025

Which two branches are you talking about?

these branches, claim and claimTo, are pretty much duplicated

i've listed in my previous comment how the .tree should look for claimTo functions, but i'll do it again—my point is to have just these tests (similarly to createWithTimestamps vs createWithDurations — we won't test again the branches we've already tested in "timestamps" in the "durations" tests) for claimTo:

├── when to address zero
│  └── it should revert
└── when to address not zero
   ├── when caller not eligible
   │  └── it should revert
   └── when caller eligible
      ├── it should mark the index as Claimed
      ├── it should transfer the ETH to the merkle lockup
      └── it should emit {Claim} event

@smol-ninja
Copy link
Member Author

smol-ninja commented May 12, 2025

I see. I understood now. Yeah that makes sense. Implemented in 96211bc.

fix: error names
docs: fix comments
build: bump solhint, evm-utils and lockup

Co-authored-by: Andrei Vlad Birgaoanu <[email protected]>
@smol-ninja
Copy link
Member Author

If no more feedback, then can you plz approve the PR after reviewing the latest commit cc @andreivladbrg?

Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

r_downey_thumbs_up

@andreivladbrg andreivladbrg merged commit c583fe6 into staging May 13, 2025
7 checks passed
@andreivladbrg andreivladbrg deleted the feat/claim-recipient branch May 13, 2025 13:27
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.

2 participants