-
Notifications
You must be signed in to change notification settings - Fork 116
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
Author Pre-Audit Merkle Distribution Contract #3148
Conversation
a1f23b5
to
eead0ad
Compare
fa99872
to
d0f3392
Compare
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 didn't review all files since it seems like many are copied. Which ones do you want review for right now?
The smart contract and test files. Everything else is still WIP for the integration interface |
d0f3392
to
fc9a9e1
Compare
1b08563
to
f9ad964
Compare
27c6ee3
to
18984ae
Compare
bcd27a9
to
2fc75bf
Compare
* Pause a Distribution indefinitely by resetting the merkle root to zero | ||
* The distribution can be re-seeded with a new merkle root. | ||
*/ | ||
function pauseDistribution(uint _distribution) external { |
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.
should be deleteDistribution
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.
This is because we are setting the bytes to 0, even though we can effectively "undelete/unpause"
|
||
// Recorded Distributions | ||
mapping(uint => bytes32) public distributionMerkleRoots; | ||
mapping(uint => mapping(address => bool)) public claimed; |
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.
idea for post-expiry admin claims: after a configured timespan passes, the admin can reclaim unclaimed funds by users.
a30156f
to
944c8e3
Compare
role. Responsible for publishing distributions on chain using the | ||
`seedAllocations` function. | ||
- `PAUSER_ROLE`: Manually assigned and managed by the `DEFAULT_ADMIN` | ||
role. Responsible for pausing a distribution, which can be unpaused. |
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.
Might be good to note here that pausing happens by removal, and that the pauser is also responsible for the minimum delay.
implementation. | ||
|
||
- A permissioned function to transfer undistributed funds out of the contract | ||
- A global `MIN_DELAY` state variable that the `SEEDER_ROLE` must observe |
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.
this can be taken off this list.
13b4206
to
5f5090e
Compare
748c5bf
to
6397af9
Compare
5f5090e
to
4773d2c
Compare
6397af9
to
4f23b36
Compare
4773d2c
to
2217f01
Compare
This smart contract is very similar to Balancer's merkle-redeem smart contract, plus the addition of a feature that allows for a delay before allocations are live to be claimed. The smart contract has a role system that allows for delegation of allocation publishing and pausing roles. This added functionality allows for a more decentralized workflows and potentially automation, since the powers of the account publishing allocations are tightly scoped, and there is recourse between the delay period and pausability. It might be worth adding a `minimum_delay` period that must be observed by the `SEEDER_ROLE` holder and can be conrolled by `PAUSER_ROLE` holders. Test Plan: Unit tests for the smart contract are included. The smart contract is fully covered, though coverage instrumentation hasn't been set up.
- added index details to slice getters - Remove `PAUSER` remnants - rename _week to _distribution - rename liquidityProvider to _contributor - rename `delayAllocation` to `delayDistribution` since claims are technically what is being delayed
Change The Delay to a Pause -- The Delay is preserved for the Seed Role and now a global minimum delay must be observed when calling `seedDistribution` Add Delay Setter -- The PAUSER_ROLE has the ability to modify the global `minDelay` in the event that the distribution process changes. test plan: unit tests added
Using 'merkletreejs' will allow us to utilize the latest, most secure MerkleTree JS implementations. This is a widely used library for many crypto projects. The API is very similar, but the construction is much more versatile and requires some specific configuration, as reflected in the changes to tests. test plan: unit tests have been updated to utilize this new dependency.
Functionally, the method deletes the hash and is therefore not just pausing the ability to claim against it. test plan: unit tests updated
2217f01
to
ada3a23
Compare
This smart contract is very similar to Balancer's merkle-redeem smart
contract, plus the addition of a feature that allows for a delay before
allocations are live to be claimed.
The smart contract has a role system that allows for delegation of
allocation publishing and pausing roles.
This added functionality allows for more decentralized workflows and
potentially automation, since the powers of the account publishing
allocations are tightly scoped, and there is recourse with a delay period.
It might be worth adding a
minimum_delay
period that must be observedby the
SEEDER_ROLE
holder and can be controlled byPAUSER_ROLE
holders.
Test Plan:
Unit tests for the smart contract are included.
The smart contract is fully covered, though coverage instrumentation
hasn't been set up.
Merge Plan
builds toward #3139