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 26205235903Coverage decreased (-38.3%) to 47.633%Details
Uncovered Changes
Coverage Regressions88556 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!
| )) | ||
| )) | ||
|
|
||
| (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.
The trait changed, which meant the tests needed to be updated.
Instead, calling it `settle-rewards`, and renaming related map to `signer-rewards-per-token-settled-for-cycle`.
|
|
rafa-stacks
left a comment
rafa-stacks
left a comment
There was a problem hiding this comment.
Thanks Hank this is very helpful for me to get started with print event ingestion. I do notice there are a few that have not yet been created, though. I added a couple comments but could you please do a pass on all state-changing functions and emit print events for all of them?
| ;; participate in the bond. | ||
| ;; | ||
| ;; This function can only be called once. | ||
| (define-public (setup-bond |
There was a problem hiding this comment.
Could you emit a print event here when a bond is created successfully?
| ;; | ||
| ;; The caller must either provide sBTC that they want to lockup, | ||
| ;; or they must provide proof of their L1 BTC lockup. | ||
| (define-public (register-for-bond |
There was a problem hiding this comment.
Also here, can you emit a print when a principal registers successfully?
|
I'm going to merge these changes and then we will continue to build in |
brice-stacks
merged commit f55fb4e
into
stacks-network:pox-wf-integration
5 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.