extendLock #1
extendLock #1
Conversation
|
General comment - this includes both efficiency improvements and new behavior. It would make the ultimate approvers job easier if these were separate PRs, so that the new desired behaviour is clear and a minimal change. |
| return _withdrawLocked(msg.sender, destination_address, kek_id, arr_idx); | ||
| } | ||
|
|
||
| // Overloaded to take the arr_idx such that it can be computed off-chain. |
There was a problem hiding this comment.
Is computing the index off chain reasonable?
Is it not that the case that the array index calculated may become out of date before it is used in which case _getState () will fail with "Stake not found" ?
There was a problem hiding this comment.
Yes it could given we're now pop()'ing the item out of the array (leaving as a delete wouldn't change the arr_idx)
It was my concern too, but according to @butlerji it's ok to make it a concern of the client to handle correctly.
That's one of the reasons I left the old version too which only takes the kek_id...again up to FRAX to comment operationally I think.
There was a problem hiding this comment.
make it a concern of the client to handle correctly.
@butlerji I don't see how this would work. Would the client specifically detect the "Stake not found" error, and then know this is a reason to retry?
There was a problem hiding this comment.
I think the flow would be pretty synchronous and single-threaded for a given address.
So first start with: myLockedStakes = contract.lockedStakesOf(myAddress)
Then if the client calls withdrawLocked() then its local array should change too for any subsequent calls (or just call contract.lockedStakesOf(myAddress) again)
There was a problem hiding this comment.
I think the flow would be pretty synchronous and single-threaded for a given address.
Oh - I misunderstood. I thought the array was being used for all addresses.
@mountainpath9 Perhaps, but given these combined changes aren't too complex I think we want the FRAX team to look at it all as one. Operationally, we may be calling |
|
|
||
| // Update the rewards array | ||
| for (uint256 i = 0; i < earned_arr.length; i++){ | ||
| // Update the rewards array |
There was a problem hiding this comment.
nitpick - on open source projects, I find the min changes you can make (basically make the reviewers job so fucking easy) the quicker you can get stuff landed. As you build up trust/rep you can start moving stuff around a bit more.
With that perspective, when putting this PR up as a draft for travis, I'd add comments calling out that this is just some loop fusion to make things a bit more efficient (possibly required to keep overall size down), and can be reverted if he prefers for simplicity.
Basically, call out explicitly what's nice to have vs the core change that must land :)
| sync_gauge_weights(false); | ||
|
|
||
| // Update the fraxPerLPStored | ||
| fraxPerLPStored = fraxPerLPToken(); |
There was a problem hiding this comment.
Again, not super obvious this code change is simply algebra moving terms around vs changing the actual implementation of something.
If just algebra, when sharing with Travis, post what you think this achieves/why it's worth the risk of change
| // | ||
| // For discussion: Do we need to keep this older version (on-chain arr_idx lookup)? | ||
| // Is overload ok, or better to have a v2? | ||
| function lockAdditional(bytes32 kek_id, uint256 addl_liq) public { |
There was a problem hiding this comment.
For what we are after - we only care about refreshing locks (as we'll never have more than a single active lock).
Do we leave this out w.r.t optimising getting our refreshing changes in (maybe similar to the above - keep calling out this is a nice to have)
| } | ||
|
|
||
| // Extend the lock period on an existing stake by secs | ||
| function extendLock(bytes32 kek_id, uint256 arr_idx, uint256 secs) updateRewardAndBalance(msg.sender, true) public returns (bytes32) { |
There was a problem hiding this comment.
IMHO I'd be partial to lockAdditional being overloaded with an extend secs
No description provided.