feat: hook allowlist #951
Merged
feat: hook allowlist #951
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
refactor: delete renounce hook test: improve test function names test: update renounce and withdraw hook tests
refactor: rename "SablierV2Recipient" to "SablierRecipient"
feat: isAllowedToHook feat: allowToHook test: rename recipient hooks mocks
refactor: remove try/ catch in hooks refactor: rename "_allowed" to "_allowedToHook" test: update cancel and withdraw tests
There was a problem hiding this comment.
Great work PRB, and thank you for moving so fast.
I have reviewed only src so far, I've not looked at tests.
I have some questions regarding the decisions made on the implementation (maybe they will be answered in the upcoming discussion you'll open):
- Since the contracts which the hook is called on must be allowed by Sablier, why not generalize it and allow both sender/recipient calls. Thus, instead of
ISablierRecipientto have aISablierLockupHookImplementerinterface?- If we keep the current name, why not rename it to
ISablierLockupRecipient? we might have a different recipient interface for future contracts
- If we keep the current name, why not rename it to
- Merge
Adminableand allow hook logic into a single contract, so thatAdminablecontract can be used accros all Sablier future contracts.- If we do this, it would be better to have in
allowToHookfunction an address param instead of an interface
- If we do this, it would be better to have in
- Why not return a bool in hook functions and check it?
|
Thanks for the kind words, @andreivladbrg. I will respond to your questions in the separate discussion thread. |
|
@andreivladbrg about your question: Why not return a bool in hook functions and check it? This answer from stack overflow put it really well 
|
fair enough, thanks for sharing |
smol-ninja
requested changes
Jun 22, 2024
There was a problem hiding this comment.
@andreivladbrg @PaulRBerg I left some comments. LMK if all looks good (you can reach with emoji). If yes I will make those changes.
smol-ninja
requested changes
Jun 22, 2024
There was a problem hiding this comment.
Reviewed tests. Left feedback below. Addressed them in 02fcc7e.
test/integration/concrete/nft-descriptor/generateAccentColor.t.sol
Outdated
Show resolved
Hide resolved
test/integration/concrete/lockup/allow-to-hook/allowToHook.t.sol
Outdated
Show resolved
Hide resolved
|
Can we all please give it a final review and approve the PR so that it can be merged? cc @PaulRBerg @andreivladbrg |
|
The PR looks good to me now. Thanks both for your contributions. Let's squash and merge! |
smol-ninja
approved these changes
Jun 24, 2024
Merged
andreivladbrg
added a commit
that referenced
this pull request
Jul 3, 2024
* refactor: delete sender withdraw hooks refactor: delete renounce hook test: improve test function names test: update renounce and withdraw hook tests * refactor: rename hooks * refactor: get rid of "hooks" directory * feat: add marker "IS_SABLIER_RECIPIENT" refactor: rename "SablierV2Recipient" to "SablierRecipient" * feat: hook allowlist feat: isAllowedToHook feat: allowToHook test: rename recipient hooks mocks * feat: check allowlist when running hooks refactor: remove try/ catch in hooks refactor: rename "_allowed" to "_allowedToHook" test: update cancel and withdraw tests * build: update precompiles * fix: remove stale dir in prepare-artifacts script * docs: update withdraw natspec * chore: address PR feedback * feat: include function selector in the return value of hook calls * fix: redundant dev notes, use interface selector * test: fix bugs * feat: replace IS_SABLIER_RECIPIENT with IERC165-supportsInterface * refactor: remove local import from Errors * docs: polish natspec * refactor: rename recipient interface * refactor: change param type to "address" * build: update precompiles * refactor: remove unneeded address casting * docs: polish natspec * chore: update count map in Base script * chore: update benchmarks * docs: mention allowlist contract in Security doc * docs: update wording --------- Co-authored-by: andreivladbrg <[email protected]> Co-authored-by: smol-ninja <[email protected]>
andreivladbrg
added a commit
that referenced
this pull request
Jul 3, 2024
* refactor: delete sender withdraw hooks refactor: delete renounce hook test: improve test function names test: update renounce and withdraw hook tests * refactor: rename hooks * refactor: get rid of "hooks" directory * feat: add marker "IS_SABLIER_RECIPIENT" refactor: rename "SablierV2Recipient" to "SablierRecipient" * feat: hook allowlist feat: isAllowedToHook feat: allowToHook test: rename recipient hooks mocks * feat: check allowlist when running hooks refactor: remove try/ catch in hooks refactor: rename "_allowed" to "_allowedToHook" test: update cancel and withdraw tests * build: update precompiles * fix: remove stale dir in prepare-artifacts script * docs: update withdraw natspec * chore: address PR feedback * feat: include function selector in the return value of hook calls * fix: redundant dev notes, use interface selector * test: fix bugs * feat: replace IS_SABLIER_RECIPIENT with IERC165-supportsInterface * refactor: remove local import from Errors * docs: polish natspec * refactor: rename recipient interface * refactor: change param type to "address" * build: update precompiles * refactor: remove unneeded address casting * docs: polish natspec * chore: update count map in Base script * chore: update benchmarks * docs: mention allowlist contract in Security doc * docs: update wording --------- Co-authored-by: andreivladbrg <[email protected]> Co-authored-by: smol-ninja <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses M-03: The caller of withdraw and renounce can skip callbacks, by sending less gas.
See related discussion: #952