feat: sablier fee on Airstreams claim #1038

smol-ninja · 2024-09-09T09:42:40Z

Changelog

Closes Modify "claim" function in MerkleLockup to charge an ETH fee #1032
Closes Override default fee in Airstreams with two mappings #1040
- SablierFee struct
Adds a new user: campaignOwner in struct Users
Adds BUSL-1.1 license to SablierMerkleFactory

andreivladbrg

i've seen that you have modified the contract as per my suggestion

i do have one more question:

src/periphery/abstracts/SablierMerkleBase.sol

test: setSablierFee

test: ReceiveEth mock contract test: update precompiles refactor: add missing override modifier refactor: return feeAmount in withdrawFees in MerkleBase

smol-ninja · 2024-09-10T12:30:43Z

This PR is now ready for review pending conclusion in #1039.

PaulRBerg · 2024-09-10T16:06:58Z

Thanks, @smol-ninja!

We need to implement some additional functionality, unfortunately.

#1040

smol-ninja · 2024-09-10T16:14:13Z

We need to implement some additional functionality, unfortunately

No problem. ~~How do we proceed @andreivladbrg? Should I close this PR then or should we review and merge it and then work on top of it?~~ #1040 (comment)

smol-ninja · 2024-09-11T16:45:10Z

@andreivladbrg You can review this PR now. Its completed from my end.

smol-ninja · 2024-10-07T20:12:43Z

Bumping this up @andreivladbrg so that you can add it to your to do task.

andreivladbrg · 2024-10-08T13:52:57Z

I’m sorry, @smol-ninja 😞. I rebased on staging from main, and it caused a lot of conflicts in this PR. I didn’t realize the consequences. I will help you fixing them

PaulRBerg · 2024-10-08T13:57:17Z

git reflog

smol-ninja · 2024-10-08T16:20:51Z

I’m sorry, @smol-ninja 😞. I rebased on staging from main, and it caused a lot of conflicts in this PR. I didn’t realize the consequences. I will help you fixing them

No problem @andreivladbrg. I am sure it wont take more than 2 mins to fix it. These ain't as scary as they look in Github UI.

andreivladbrg

So sorry for the late review, we’ve been busy with Flow, and I’ve just gotten my focus back on Lockup.

I haven’t fully reviewed the tests yet, but I’ll leave my feedback on src below:

Also, why campaignOwner and not campaignAdmin?

src/periphery/interfaces/ISablierMerkleFactory.sol

src/periphery/SablierMerkleFactory.sol

src/periphery/SablierMerkleInstant.sol

src/periphery/abstracts/SablierMerkleBase.sol

src/periphery/interfaces/ISablierMerkleFactory.sol

src/periphery/interfaces/ISablierMerkleBase.sol

src/periphery/interfaces/ISablierMerkleFactory.sol

smol-ninja · 2024-10-12T22:24:12Z

Finally. Thanks for the partial review @andreivladbrg. Appreciate it!

Also, why campaignOwner and not campaignAdmin?

No reason just #1032 (comment): campaignAdmin didn't come to my mind when I was proposing this. Paul was fine with anything as long as it is prefixed with campaign and you didn't object as well.

refactor: use plural in mapping names refactor: DRY'ify through _computeSablierFeeForUser function refactor: adds missing override refactor: rename SetSablierFee to SetSablierFeeForUser feat: add non-zero check for to address in withdrawFees docs: polish natspecs fix: typos Co-authored-by: Andrei Vlad Birgaoanu <99738872+andreivladbrg@users.noreply.github.com>

smol-ninja · 2024-10-13T16:27:35Z

As per Slack conversation between @razgraf and @andreivladbrg, its beneficial for the UI to include sablierFee in the CreateMerkle events. However, to be able to include sablierFee in CreateMerkleLT event, I would either have to merge _deployMerkleLT code into createMerkleLT or return sablierFee in _deployMerkleLT to then access it in the event log.

@andreivladbrg, which one would you go for? I’d like to go with the first approach as _deployMerkleLT is anyways only called in createMerkleLT so why not include the code directly and avoid having returned value. But I do not remember why we decided to have a separate function _deployMerkleLT.

src/core/abstracts/SablierLockup.sol

src/periphery/SablierMerkleFactory.sol

andreivladbrg

2 more comments re src

andreivladbrg

I just reviewed the tests as well, and they generally look good. (pushed some small polishes cf0990e)

There are a couple of things I would update (due to the new shared structure in periphery), but to avoid doing the same work twice after rebasing in this PR, I’ll address them there.

Sorry again for the long time waited. We are finally here 🚀

test/Base.t.sol

test/periphery/fork/merkle-campaign/MerkleInstant.t.sol

test/periphery/fork/merkle-campaign/MerkleLL.t.sol

test/periphery/fork/merkle-campaign/MerkleLT.t.sol

docs: polish natspecs over setDefaultSablierFee chore: change visibility to private for internal functions in Factory

andreivladbrg · 2024-10-24T14:13:23Z

@andreivladbrg, which one would you go for? I’d like to go with the first approach as _deployMerkleLT is anyways only called in createMerkleLT so why not include the code directly and avoid having returned value. But I do not remember why we decided to have a separate function _deployMerkleLT.

@smol-ninja I have missed this one.
Do what you think is best.

The reason I introduced the internal function for deployment was due to a "stack too deep" error. If you can manage to abstract the logic into a single function, feel free to go for it.

smol-ninja marked this pull request as draft September 9, 2024 09:42

feat: receive ether in claim function

c010a67

smol-ninja force-pushed the feat/claim-fee-airstream branch from de15007 to 945477a Compare September 9, 2024 21:00

andreivladbrg reviewed Sep 9, 2024

View reviewed changes

src/periphery/abstracts/SablierMerkleBase.sol Outdated Show resolved Hide resolved

test: msg.value in claim function

357e720

test: setSablierFee

smol-ninja force-pushed the feat/claim-fee-airstream branch from 4657ecc to 4abe924 Compare September 9, 2024 23:24

smol-ninja added 2 commits September 10, 2024 00:31

test: fix claim in fork tests

d90bd2c

refactor: remove redundant receive function

0f2985a

andreivladbrg mentioned this pull request Sep 10, 2024

Modify "claim" function in MerkleLockup to charge an ETH fee #1032

Closed

5 tasks

test: withdrawFees in Factory and MerkleBase

d135b8b

test: ReceiveEth mock contract test: update precompiles refactor: add missing override modifier refactor: return feeAmount in withdrawFees in MerkleBase

smol-ninja marked this pull request as ready for review September 10, 2024 10:57

smol-ninja force-pushed the feat/claim-fee-airstream branch from fdfb966 to 475bf14 Compare September 10, 2024 10:59

smol-ninja added 3 commits September 10, 2024 13:07

refactor: include merkleLockup address in WithdrawSablierFees event

6eefaf9

test: withdraw-fees in fork tests

3135886

chore: add BUSL license to factory contract

90ca9d0

smol-ninja requested a review from andreivladbrg September 10, 2024 12:27

This comment was marked as outdated.

Sign in to view

smol-ninja mentioned this pull request Sep 10, 2024

Override default fee in Airstreams with two mappings #1040

Closed

4 tasks

smol-ninja added 3 commits September 11, 2024 12:22

refactor: rename sablierFee to defaultSablierFee in factory

394cf27

feat: custom sablier fee

a582341

test(refactor): add a new user: campaignOwner in User struct

d019b8d

smol-ninja changed the title ~~feat: receive ether in claim function~~ feat: sablier fee on airstreams claim Sep 11, 2024

smol-ninja changed the title ~~feat: sablier fee on airstreams claim~~ feat: sablier fee on Airstreams claim Sep 11, 2024

smol-ninja marked this pull request as draft September 11, 2024 14:38

andreivladbrg force-pushed the staging branch from 2534f59 to 53ef500 Compare September 11, 2024 16:17

smol-ninja added 2 commits September 11, 2024 17:21

test: custom sablier fee for users

6ec1142

style: fix import

bc77b3c

smol-ninja force-pushed the feat/claim-fee-airstream branch from f727f8e to b82c687 Compare September 11, 2024 16:42

smol-ninja requested a review from PaulRBerg September 16, 2024 20:49

andreivladbrg force-pushed the staging branch from 937c14d to e82b2de Compare October 8, 2024 13:41

smol-ninja force-pushed the feat/claim-fee-airstream branch 3 times, most recently from 974cac0 to de0bce6 Compare October 8, 2024 18:25

style: lint code

2538668

smol-ninja force-pushed the feat/claim-fee-airstream branch from de0bce6 to 2538668 Compare October 11, 2024 06:38

andreivladbrg requested changes Oct 12, 2024

View reviewed changes

docs: replace caller with msg.sender

155401c

smol-ninja requested a review from andreivladbrg October 20, 2024 14:07

andreivladbrg reviewed Oct 22, 2024

View reviewed changes

src/core/abstracts/SablierLockup.sol Show resolved Hide resolved

src/periphery/SablierMerkleFactory.sol Show resolved Hide resolved

src/periphery/SablierMerkleFactory.sol Outdated Show resolved Hide resolved

andreivladbrg reviewed Oct 22, 2024

View reviewed changes

small polishes

cf0990e

andreivladbrg approved these changes Oct 23, 2024

View reviewed changes

test: dry'ify vm.expectCall

ce35add

docs: polish natspecs over setDefaultSablierFee chore: change visibility to private for internal functions in Factory

smol-ninja added 3 commits October 24, 2024 15:46

feat: emit sablierFee in CreateMerkle events

37c7caf

test: move periphery related helpers to Periphery base contract

72af0c3

docs: remove redundant comment from constructors

5b24e38

smol-ninja merged commit 9974448 into staging Oct 24, 2024
9 checks passed

smol-ninja deleted the feat/claim-fee-airstream branch October 24, 2024 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: sablier fee on Airstreams claim #1038

feat: sablier fee on Airstreams claim #1038

smol-ninja commented Sep 9, 2024 •

edited

Loading

andreivladbrg left a comment

smol-ninja commented Sep 10, 2024

PaulRBerg commented Sep 10, 2024 •

edited by smol-ninja

Loading

smol-ninja commented Sep 10, 2024 •

edited

Loading

This comment was marked as outdated.

smol-ninja commented Sep 11, 2024

smol-ninja commented Oct 7, 2024

andreivladbrg commented Oct 8, 2024

PaulRBerg commented Oct 8, 2024

smol-ninja commented Oct 8, 2024

andreivladbrg left a comment

smol-ninja commented Oct 12, 2024

smol-ninja commented Oct 13, 2024

andreivladbrg left a comment

andreivladbrg left a comment

andreivladbrg commented Oct 24, 2024

feat: sablier fee on Airstreams claim #1038

feat: sablier fee on Airstreams claim #1038

Conversation

smol-ninja commented Sep 9, 2024 • edited Loading

Changelog

andreivladbrg left a comment

Choose a reason for hiding this comment

smol-ninja commented Sep 10, 2024

PaulRBerg commented Sep 10, 2024 • edited by smol-ninja Loading

smol-ninja commented Sep 10, 2024 • edited Loading

This comment was marked as outdated.

smol-ninja commented Sep 11, 2024

smol-ninja commented Oct 7, 2024

andreivladbrg commented Oct 8, 2024

PaulRBerg commented Oct 8, 2024

smol-ninja commented Oct 8, 2024

andreivladbrg left a comment

Choose a reason for hiding this comment

smol-ninja commented Oct 12, 2024

smol-ninja commented Oct 13, 2024

andreivladbrg left a comment

Choose a reason for hiding this comment

andreivladbrg left a comment

Choose a reason for hiding this comment

andreivladbrg commented Oct 24, 2024

smol-ninja commented Sep 9, 2024 •

edited

Loading

PaulRBerg commented Sep 10, 2024 •

edited by smol-ninja

Loading

smol-ninja commented Sep 10, 2024 •

edited

Loading