-
Notifications
You must be signed in to change notification settings - Fork 2
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
refactor: moving merkle lockup contracts to airdrops repo #5
Conversation
Co-authored-by: Andrei Vlad Birgaoanu <99738872+andreivladbrg@users.noreply.github.com>
test: add unlockAmounts assertions in claim test: make START_AMOUNT non-zero in defaults
I think we should have invariants and fuzz tests for Airdrops repo in the future. |
|
|
I see. The overhead for using the precompiles flow you described instead of |
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.
Good work @smol-ninja. Feedback below.
I've pushed two commits:
- One to address the feedback below and other non-contentious changes.
- Another to rename "Sablier Fee" to just "Fee", and "Sablier Fee By User" to "Custom Fee"
LMK what 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.
I saw @PaulRBerg left some comments.
Leaving these as well for now, but I have not reviewed the tests
chore: remove SVG references chore: update ignore and config files docs: extend BUSL license docs: improve writing docs: roll v1.3.0 refactor: alphabetical ordering test: remove unused constants
refactor: rename "Sablier Fee By User" to "Custom Fee"
Thanks for the reviews. Everything looks good to me. LMK if its good to merge or if there are any more changes. |
Co-authored-by: Paul Razvan Berg <prberg@proton.me>
Great job. |
Changelog
test
dir totests
(plural) #3Some notes
Precompiles could not be used to deploy Lockup protocol due to linked libraries. The Lockup bytecode requires to replace libraries' placeholders with the actual addresses to which libraries are deployed through precompiles. However, replacing bytecode string is not possible without having access to
vm
insidePrecompiles
contract. The same can also not be achiveed by the bash script because the libraries can only be deployed whendeployCore
is called.Inspite of exposing
utils
in Lockup npm package, I decided to create a separateutils
forairdrops
. My rationale is thatairdrops
aims to be a separate protocol which partially depends on the Lockup contract. Hence, having its own utility contracts would allow us to add/update the code with minimum dependency over Lockup. Otherwise a change in here may require us to update it in Lockup first.The lockup dependency in this PR is unlock-amounts branch which should be changed to
staging
once the unlock-amount PR is merged.I have picked
Schedule
related changes from feat: add unlock amounts functionality lockup#1075.Given that v2.0.0 will be its first release as a separate repo, we can have
main
treated asstaging
for this repo (similar to how we did in Flow).Tips on reviewing this PR
My recommendation would be to read the repo as if its a new code. This will help you catch any errors and redundant codes.