Delegation (Experimental) #378
Conversation
l-monninger
requested review from
andyjsbell,
0xmovses and
mzabaluev
as code owners
| // for every delegatee of the attester | ||
| for ( | ||
| uint256 k = 0; | ||
| k < delegatorsByAttesterByDomain[domain][attester].length(); |
There was a problem hiding this comment.
I don't know how good the optimizer is, but perhaps getting the delegatorsByAttesterByDomain[domain][attester] reference for the duration of the loop would save some gas.
There was a problem hiding this comment.
Yeah, I think it will actually save some gas.
| epochStakesByDomain[domain][epoch][custodian][attester][ | ||
| delegator | ||
| ] -= amount; | ||
|
|
||
| if ( | ||
| epochStakesByDomain[domain][epoch][custodian][attester][ | ||
| delegator | ||
| ] == 0 | ||
| ) { |
There was a problem hiding this comment.
Here as well: the value has just been decremented, could we hold on to the reference and get the value instead of spending gas on 5 nested lookups another time?
There was a problem hiding this comment.
Yep, good call.
| EnumerableSet.AddressSet storage attesters = attestersByDomain[domain]; | ||
|
|
||
| for (uint256 i = 0; i < attestersByDomain[domain].length(); i++) { | ||
| address attester = attestersByDomain[domain].at(i); | ||
| for (uint256 i = 0; i < attesters; i++) { | ||
| address attester = attesters.at(i); | ||
|
|
||
| for (uint256 j = 0; j < custodiansByDomain[domain].length(); j++) { | ||
| address custodian = custodiansByDomain[domain].at(j); | ||
| // for every custodian | ||
| EnumerableSet.AddressSet storage custodians = custodiansByDomain[ | ||
| domain | ||
| ]; | ||
| for (uint256 j = 0; j < custodians; j++) { | ||
| address custodian = custodians.at(j); | ||
|
|
||
| // for every delegatee of the attester | ||
| for ( | ||
| uint256 k = 0; | ||
| k < delegatorsByAttesterByDomain[domain][attester].length(); | ||
| k++ | ||
| ) { | ||
| // todo: can this be replaced with _rollOverAttester? | ||
| address delegatee = delegatorsByAttesterByDomain[domain][ | ||
| EnumberableSet.AddressSet | ||
| storage delegatees = delegatorsByAttesterByDomain[domain][ |
There was a problem hiding this comment.
Looks much better, aesthentically and I assume also gas-wise. I'm not an expert on Solidity, though.
| uint256 stake = epochStakesByDomain[domain][epoch][custodian][attester][ | ||
| delegator | ||
| ]; | ||
| stake -= amount; // okay to let this error if it underflows | ||
| epochStakesByDomain[domain][epoch][custodian][attester][ | ||
| delegator | ||
| ] -= amount; | ||
| ] = stake; |
There was a problem hiding this comment.
You could still use a storage reference here? This change only replaces one 5-step lookup for another.
There was a problem hiding this comment.
This is outdated, but I was doing this to save the extra stake lookups later on. You do also in theory save one instruction in the bytecode here.
| MovementStaking staking = new MovementStaking(); | ||
| staking.initialize(moveToken); |
There was a problem hiding this comment.
I would advise against building tests that initalize the implementation contract and not a proxy contract. This might lead to clash in functions or impredictable behavior. There has been recently a hack on deltaprime that was caused by this.
I'd be glad to redesign these tests.
There was a problem hiding this comment.
That's probably best for a separate PR as literally all of the tests use this approach.
There was a problem hiding this comment.
Can you provide some links here? It would seem to me you'd have to be doing something exotic with your use of the proxy pattern to actually have behavior vary in ways that tests like these wouldn't assert.
There was a problem hiding this comment.
Or, in the least, you would not need to check all of the logic and instead access controls.
There was a problem hiding this comment.
https://medium.com/@Delta_Prime/deltaprime-post-mortem-report-752bd60a25e6
I think it's a bit further than what I expected but I think that anything that gets us closer to the actual implementation might prevent us from running into a critical bug in production.
| delegator | ||
| ]; | ||
| stake -= amount; // okay to let this error if it underflows | ||
| epochStakesByDomain[domain][epoch][custodian][attester][ |
There was a problem hiding this comment.
funds get stuck if it underflows, better handle it with "if amount > _stake, _stake = 0"
| for (uint256 k = 0; k < delegatees.length(); k++) { | ||
| // todo: can this be replaced with _rollOverAttester? | ||
| address delegatee = delegatees.at(k); | ||
|
|
There was a problem hiding this comment.
to use _rollOverAttester, it needs to start using nextEpoch param instead of currentEpoch. This way 0 can be passed as the param. This requires a modification of the rest of the code, but definitely seems okay.
Just rolling with the current implementation basicTest passes, it requires a test to verify if the correct epoch has been settled.
|
|
||
| // roll over the genesis stake to the current epoch | ||
| _addStake(domain, getCurrentEpoch(domain), custodian, delegatee, attester, attesterStake); | ||
| } | ||
| } |
There was a problem hiding this comment.
If _rollOverAttester is to be used, it might be optimized to take storage params instead
…ninger/delegation-experimental
0xPrimata
force-pushed
the
l-monninger/delegation-experimental
branch
from
dc712ff
to
19b947c
Compare
Summary
protocol-unitsChangelog
Testing
testBasicDelegationunit test covers staking and unstaking with delegation.Outstanding issues