WIP: PoX-5 and Epoch 4#7197
Conversation
| ;; this is a list of outputs corresponding to their timelocks. | ||
| ;; If the response is `err`, this is the amount of sBTC (in sats) | ||
| ;; that they want to lock. | ||
| (btc-lockup (response { |
There was a problem hiding this comment.
I think this being a response is bad for UX. I'd rather see two optionals, btc-lockup and sbtc-lockup, with a check to ensure they are not both set.
There was a problem hiding this comment.
Yeah, I certainly see what you mean, but in general I like a compiler check more than a runtime check. I do agree it's not the best UX, but I'm not sure having two optionals is either.
| (ok { | ||
| unlock-burn-height: (reward-cycle-to-unlock-height unlock-cycle), | ||
| staker: tx-sender, | ||
| signer: signer, | ||
| prev-unlock-height: prev-unlock-cycle, | ||
| unlock-cycle: unlock-cycle, | ||
| num-cycles: num-cycles, | ||
| amount-ustx: new-lock-amount, | ||
| }) |
There was a problem hiding this comment.
Again, field ordering in tuples is nice to be consistent when possible.
(ok {
- unlock-burn-height: (reward-cycle-to-unlock-height unlock-cycle),
- staker: tx-sender,
signer: signer,
+ staker: tx-sender,
+ amount-ustx: new-lock-amount,
+ num-cycles: num-cycles,
prev-unlock-height: prev-unlock-cycle,
+ unlock-burn-height: (reward-cycle-to-unlock-height unlock-cycle),
unlock-cycle: unlock-cycle,
- num-cycles: num-cycles,
- amount-ustx: new-lock-amount,
})
)
)| ) | ||
| ) | ||
|
|
||
| (define-public (calculate-rewards (bond-periods (list 6 uint))) |
There was a problem hiding this comment.
It might be nice to have a read-only version of this that could be called any time to estimate rewards, and then call that from the actual one that checks the proper height and sets the persistent storage.
|
|
||
| ;; Used to prevent fractional multiplication errors | ||
| ;; during reward calculations | ||
| (define-constant PRECISION u1000000000000000000) ;; 1e18 |
There was a problem hiding this comment.
It will be better performance for this to be a power of 2 (multiplies and divides become shifts).
| ;; normally, stacking methods may only be invoked by _direct_ transactions | ||
| ;; (i.e., the tx-sender issues a direct contract-call to the stacking methods) | ||
| ;; by issuing an allowance, the tx-sender may call through the allowed contract | ||
| (define-public (allow-contract-caller |
There was a problem hiding this comment.
Are we still going to consider the post-condition replacement for this? I think it would be nice. See discussion.
| index: uint, | ||
| is-bond: bool, | ||
| staker: principal, | ||
| signer: principal, |
There was a problem hiding this comment.
Is it necessary to include the signer in the key here? I think we're already guaranteed to only have one entry per index/is-bond/staker, and adding the signer is just complicating it.
There was a problem hiding this comment.
Yeah you're right about uniqueness, but it also acts as a kind of lookup variable, where we do want to answer "for this signer, how many shares ...". And so to answer that, you'd have to do either 2 queries, or add another field to the value. I can see arguments for putting signer in the value, though.
|
|
| Ok(SignerCalculation { | ||
| reward_set: RewardSet::Waterfall(WaterfallCycleSet { | ||
| sbtc_address, | ||
| signers: signer_set, |
There was a problem hiding this comment.
Shouldn't this fail if signer_set.is_empty()? pox_5_stake_entries can return an empty iterator when PoX-5 has no first signer for the
cycle, but this still stores RewardSet::Waterfall { signers: [] }. Since Waterfall signers() returns Some([]), later signature
validation computes total weight/threshold as 0 and accepts blocks with no signer signatures.
| (accumulation (try! (fold validate-l1-lockup (get outputs lockups) | ||
| (ok { | ||
| sum: u0, | ||
| expected-script-hash: expected-timelock-output, | ||
| }) | ||
| ))) |
There was a problem hiding this comment.
Should this reject duplicate L1 outpoints before summing? A caller can include the same {tx, output-index} multiple times in one outputs list; each entry passes validation and is added to sats-total, inflating bond shares before the single staker/bond-membership checks run.
In this scenario, instead of the reserve getting 15% of the leftover rewards, it gets all of the leftovers, since there are no STX-only stakers to pay.
merge latest develop
Bump clarinet-sdk to 3.19.0 with lock-aware boot pox-5
This behavior at rollover time has been flagged multiple times by reviewers, so this PR just adds some comments to clarify the intention and another test to verify the behavior. The issue is that when rolling over (bond->bond or stx-only->bond or bond->stx-only), the action is allowed during the last half of the last cycle (the same timing as the L1 unlock), but the rewards for that last cycle are unaffected.
fix: set /v2/pox's minimum to the hard-coded pox-5 value
fix: reject unstake-sbtc and announce-l1-early-exit during prepare
There seems to be a bug in vitest that causes it to stall when there is that failure during setup. The latest v4 version fixes the hang, but the error is hidden. I've opened an issue with `vitest` to fix this.
This gives us the variadic `concat` support needed to merge this PR.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ng-logic Fix: current cycle unstaking logic
…-stacks/stacks-core into fix/current-cycle-unstaking-logic
…ng-logic Fix missing integration test
chore: document and verify the rollover behavior
refactor: use new variadic `concat`
…ntracts fix: bump clarigen to fix clarigen-types issue
feat: replace `allow-contract-caller` with staking and pox PCs
11 participants
This is an in-progress PR for iterative development of pox-5 work