-
Notifications
You must be signed in to change notification settings - Fork 207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
staketia migration - staketia accounting #1214
Conversation
// If this fails, do not proceed to the delegation or undelegation step | ||
// Note: This must be run first because it is used when refreshing the native token | ||
// balance in prepare undelegation | ||
if err := k.UpdateRedemptionRate(ctx); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that the ordering of refreshing the RR and sending delegations and undelegations matters. The comments here suggest the same.
If we move the update RR step to stakeibc, but keep the delegation and undelegation txs in staketia, we just need to make sure the update RR is run at the same epochly cadence.
Just to triple check - it is, right? I.e. the cadence at which we ran it here matches the cadence at which we run it in stakeibc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GREAT question! The cadence will be different in stakeibc (it runs every stride epoch instead of every day epoch)
i think how we have it right now, the RR will be ~6 hours stale when its used for unbondings here. But imo, since that's pretty negligible, I'd prefer to keep it as is, rather than mess with the ordering. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I see.
So just to map out the worst case: some packets may be are stuck or reinvestment would be otherwise delayed for a long time, then we'd see a large RR jump that triggers within the "stale period". In that case, we'd use the stale RR for unbonding and the users who receive those unbondings would not receive their yield from the "stuck packet" period. Is that right?
Just to explore syncing up the epochs.... what if we had staketia epochs run every 6 hours? I imagine not much would change in the 6-hourly logic until the operator advances the state. But in that case, would this edge case or the "stale period" be solved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that worst case is righ!, But I think it's pretty unlikely that we have some multi-day bottleneck that just happens to clear in this 6 hour window. And even in that case, the impact is low - they just get the RR at the time of redemption (similar to how it used to be done).
Running the epochs every 6 hours could work, but I'd be a bit worried we have some implicit invariant about the length and not sure it's worth the effort to investigate based on the above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even in that case, the impact is low - they just get the RR at the time of redemption (similar to how it used to be done).
Yeah good point that it'd revert to how it used to be done. We did get some complaints about that, but as long as we don't have a long packet backlog we're good. And ultimately this migration period is temporary so the time window within which we could see backlogs is bounded.
Fwiw I'm mostly convinced to leave things as is, especially because of "we [may] have some implicit invariant about the length" could lead to a serious accounting issue if overlooked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good point about this being temporary!
also I assume the complaints were more in the case where the unbondings were delayed right? I’d imagine w/o a delay, getting the same RR that’s shown in the FE at time of redemption wouldnt upset anyone!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really clean. PR description and spec helped a ton in the review, ty for making those to clear. Left a few questions.
@sampocs i think this question is still outstanding |
We don't have any tia RLs at the moment. Also RLs are module agnostic! |
I'm signed off here. Leaving this open for other reviewers to opine on. |
Context
This PR handles the main accounting changes that will take place in the staketia module during the migration. At a high level, the main changes are:
DelegatedBalance
field on the host zone is now renamed toRemainingDelegatedBalance
and is only meant to track the delegations left in the multisig account.Context on Accounting
In staketia, the
DelegatedBalance
field previously was changed in the following stages:LiquidStake
- no changeConfirmDelegation
- incrementedRedeemStake
- no changeConfirmUndelegation
- decrementedAnd in stakeibc, we similarly have the following with the
TotalDelegations
field on host zoneLiquidStake
- no changeDelegationCallback
- incrementedRedeemStake
- no changeUndelegationCallback
- decrementedAfter the migration, the replacement field
RemainingDelegatedBalance
in staketia will update as follows:LiquidStake
- no change (obviously)ConfirmDelegation
- incremented (only occurs in one delegation cycle after the upgrade - see phase 2.5 in spec)RedeemStake
- decremented so that we know when to switch over after the last token's been redeemedConfirmUndelegation
- no changeTotalDelegations
in stakeibc will be decrementedAccounting Changelog
Based on the above, there are the following changes:
ConfirmDelegation
now incrementsRemainingDelegatedBalance
instead ofDelegatedBalance
RedeemStake
(handled in separate PR)ConfirmUndelegation
now decrementsTotalDelegations
instead ofDelegatedBalance
AdjustDelegatedBalance
now referencesRemainingDelegatedBalance
instead ofDelegatedBalance
Remaining Changelog
DelegatedBalance
toRemainingDelegatedBalance
on host zone