Feat: L1 and L2 early unlocks, updating signer#7199
Conversation
Moved from tracking `rewards-paid` to `rewards-per-token-paid`. This is required to account for scenarios where a signer's staked amount changes without claiming rewards. This is the case, for example, with early L1 unlocks.
Coverage Report for CI Build 25903914664Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage decreased (-38.8%) to 47.122%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions89242 previously-covered lines in 343 files lost coverage.
Coverage Stats
💛 - Coveralls |
|
|
||
| ;; As a bond participant with locked sBTC, remove a portion (or all) | ||
| ;; of your locked sBTC. | ||
| (define-public (unstake-sbtc |
There was a problem hiding this comment.
This may be controversial, but could we avoid the term "unstake" here so it doesn't get confused with STX unstaking? I know we're saying "Bitcoin staking," but in the contract, it's register-for-bond, not stake-sbtc. Maybe this could be called early-exit-bond or something?
There was a problem hiding this comment.
it's register-for-bond, not stake-sbtc.
Right, but register-for-bond supports either sBTC or BTC, whereas this "sbtc early unlock" flow is pretty different (different caller, one allows amounts, etc). But I'm happy to call it early-exit-bond!
|
|
||
| ;; As a bond participant with locked sBTC, remove a portion (or all) | ||
| ;; of your locked sBTC. | ||
| (define-public (unstake-sbtc |
There was a problem hiding this comment.
Should this be permissioned, like the L1 early exit is?
There was a problem hiding this comment.
Totally valid question, but we've decided to not permission this
| )) | ||
| )) | ||
|
|
||
| (ok { |
There was a problem hiding this comment.
We should add a print event in here.
| ) | ||
|
|
||
| ;; As a bond participant with locked sBTC, remove a portion (or all) | ||
| ;; of your locked sBTC. |
There was a problem hiding this comment.
Can you add a note that all of the STX stays locked for the duration of the bond, regardless of any early exit?
| ) | ||
| ) | ||
|
|
||
| (define-public (announce-l1-early-exit |
There was a problem hiding this comment.
Could you add a comment here? Be sure to note that the STX remains locked.
| (define-map signer-rewards-paid-for-cycle | ||
| ;; Represents a snapshot of `rewards-per-token` at the last | ||
| ;; time of rewards calculation for this specific signer | ||
| (define-map signer-rewards-per-token-paid-for-cycle |
There was a problem hiding this comment.
This paid (and pending below) is little confusing. What if we called it signer-rewards-per-token-settled-for-cycle instead and instead of crystallize-rewards it was settle-rewards. We could also make signer-pending-rewards-for-cycle be signer-accrued-rewards-for-cycle. What do you think? Does that make it any more clear?
There was a problem hiding this comment.
Yep I am on board, especially with crystallize-rewards -> settle-rewards, and signer-rewards-per-token-settled-for-cycle. Instead of "accrued" can I suggest "unclaimed", since that's more accurate? The "pending" value is "rewards that have been settled but not transfered to the signer yet".
Previously, the maps were called `staker-set-..`, but this reflected a previous design. Now, the linked list only contains signer principals. This commit updates those maps, functions, and argument names to reflect the fact that the linked list contains the _signer_ set.
| ;; Take a snapshot of the signer's current rewards | ||
| (crystallize-rewards signer bond-index true) | ||
|
|
||
| (map-set staker-shares-staked-for-cycle { |
There was a problem hiding this comment.
Do we need to allow a bond holder to register for bond N, then early exit (through this method or on the L1 with announce-l1-early-exit), then register for another bond, before N+6? In other words, do we need a special case here for when new-amount-sats is 0, to do more cleanup?
| ;; @param target-rate; target yield rate (apy) in basis points | ||
| ;; @param stx-value-ratio; representation of STX:BTC price | ||
| ;; @param min-ustx-ratio; minimum amount of STX that must be locked | ||
| ;; relative to BTC for this term. Represented in basis points. | ||
| ;; @param early-exit-signers: An allowlist of bond members that can | ||
| ;; participate in the bond. |
There was a problem hiding this comment.
Feels like you should have a line for all parameters, not just some.
3 participants
This PR includes the ability for bond participants to unlock some of their sBTC. I've also added the hook for a bond admin to announce an L1 early unlock.
This new functionality highlighted the need for a change in how our internal state was being tracked. Previously, a signer's total shares for a cycle only changed in an upcoming cycle, so there was no way for a change of balance to effect already earned rewards. This is not the case with bond unlocks. Because of this, I've refactored rewards tracking to have a "snapshot" capability, where snapshotting a signer's rewards is separate from the actual act of claiming rewards. This is also how I've implemented things in the signer manager contract.
This also highlighted the need to add a
checkpoint-signercall that is invoked when a staker changes their signer or unlocks some of their shares. This is a new trait function, and now the relevant functions must provideold-signer-managerto allow this trait function to be invoked.