Updates User Redemption Records dynamically before unbonding #1049

shellvish · 2024-01-09T17:41:46Z

WIP - Still need to add unit tests.

High level, this change makes it so redemption rates get set at the time that unbondings are initiated, not at the time of redemption.

I would focus on a few main things:

Does it ever break any assumptions if the NativeTokenAmount on a HostZone is set to 0 before unbondings finish?
Do we handle failure cases (e.g. callback errors or times out) correctly?
Do we migrate our current data structures over properly?

riley-stride

Looking good, thanks for taking point on this one! Added some questions and suggestions.

riley-stride · 2024-01-09T23:29:49Z

x/stakeibc/keeper/unbonding_records.go

@@ -74,6 +74,14 @@ func (k Keeper) CreateEpochUnbondingRecord(ctx sdk.Context, epochNumber uint64)
 	return true
 }

+// Helper function to evaluate if a host zone unbonding record still needs to be initiated
+func (k Keeper) ShouldInitiateHostZoneUnbondingRecord(ctx sdk.Context, hostZoneRecord *recordstypes.HostZoneUnbonding) bool {
+	if hostZoneRecord.Status == recordstypes.HostZoneUnbonding_UNBONDING_QUEUE && hostZoneRecord.NativeTokenAmount.GT(sdkmath.ZeroInt()) {


Will NativeTokenAmount always be 0 now, causing this check to always fail?

🤦

Yes, you're 100% right. Great catch, fixing!

I think your fix here works, since NativeTokenAmount will (once set) equal stTokenAmount times a non-zero RR. So checking stTokenAmount for equality to 0 has the same effect as checking whether the value NativeTokenAmount will eventually be set to equals 0.

x/stakeibc/keeper/msg_server_redeem_stake.go

riley-stride · 2024-01-09T23:43:42Z

x/stakeibc/keeper/unbonding_records.go

+	hostZoneUnbondingRecord.NativeTokenAmount = totalNativeAmount
+	updatedEpochUnbondingRecord, success := k.RecordsKeeper.AddHostZoneToEpochUnbondingRecord(ctx, epochNumber, chainId, hostZoneUnbondingRecord)
+	if !success {
+		// TODO QUESTION: what should we do on failure? do we revert the userRedemptionRecord changes above?


(Low confidence) I think we should either make all the changes in this function or none of them, so would revert (error) rather than returning false.

I see in one call to UpdateNativeTokensForHostZoneUnbondingRecord you throw an error upstream here. However in another call to it here you don't throw an error upstreadm (afaict).

I think we want to revert changes always so would maybe suggest throwing the error inside UpdateNativeTokensForHostZoneUnbondingRecord so that future engineers don't have to think about throwing errors on UpdateNativeTokensForHostZoneUnbondingRecord failure.

What might cause this function to fail? Only case I can see is if GetEpochUnbondingRecord fails to find the record, which seems impossible given it's passed in from an EpochUnbondingRecord as epochUnbonding.EpochNumber

Yeah that makes sense! The reason I threw an error in the first location is because we need that function call to succeed (otherwise we don't know how many tokens to unbond), but we don't strictly need it in the other function call (it's only used for housekeeping niceness).

That being said, I lean towards throwing an error regardless. I put the tradeoffs in here, my recommendation is we get one-notch more confident that the function won't fail, and then we choose to always revert.

Added a comments below at the link. (Low confidence) I think the records could get out of sync if we don't revert. Agree we should get one-notch more confident that the function won't fail, and then we choose to always revert.

Even if we throw an error, state doesn't get reverted since this is in the begin blocker. We should use a cache context wrapper around this function and throw an error

it's not necessarily in the begin blocker - re-opening ICAs is just a tx

riley-stride · 2024-01-09T23:51:24Z

x/stakeibc/keeper/unbonding_records.go

+			return errorsmod.Wrapf(types.ErrHostZoneNotFound, errMsg)
+		}
+		success := k.UpdateNativeTokensForHostZoneUnbondingRecord(ctx, epochUnbondingRecordId, hostZoneUnbonding, true)
+		if !success {


Should we throw an error here to revert state if UpdateNativeTokensForHostZoneUnbondingRecord fails, or maybe throw an error within UpdateNativeTokensForHostZoneUnbondingRecord if it fails?

I was wondering this as well, I think the tradeoff is

Currently, we don't actually need the output of UpdateNativeTokensForHostZoneUnbondingRecord to succeed, it's mainly housekeeping to keep the records neat. If this failed for whatever reason, it would be nice to not halt the chain / break critical functionality

But... in the future it's very possible that we will rely on this state, and then we absolutely do need to revert state if this failed. It's very possible that by not reverting it now, we won't catch an error, and then the impact will be much more severe in the future.

I don't feel strongly on which path we take, could easily be convinced to revert state or just log an error. Curious what you, @asalzmann , and @sampocs think!

Also, realistically I don't think this function should fail at all. imo if we all looked hard at this function for a few minutes and were confident it shouldn't error, I think option (2) is a no-brainer!

Currently, we don't actually need the output of UpdateNativeTokensForHostZoneUnbondingRecord to succeed, it's mainly housekeeping to keep the records neat. If this failed for whatever reason, it would be nice to not halt the chain / break critical functionality

If the highligted line failed (the call to AddHostZoneToEpochUnbondingRecord) but we didn't revert, would we end up in a bad state?

Afaict the totalNativeAmount would have been incremented and written to the URR a few lines prior, but the corresponding HZUR would not have been updated, so the two would be out of sync.

Mentioned in the other thread as well - state does not get reverted in the begin/end blocker even if we return an error. We should wrap this in a temporary context so we can discard state changes if an error is thrown. I believe this is already implemented in an old PR

Ah yeah that's a great point Riley! Agreed, I think these will get out-of-sync the way it's currently written

Sam, do you have an example of the temporary context? I can implement it here, I think that's likely the best path forward

https://github.com/Stride-Labs/stride/pull/861/files#diff-ba71e9ef6bf1ebcc5243683cbd44405947d8109d7ffb825c7c353d256f7e2179R355-R358

same comment as above - this is called outside of the begin blocker (acks, tx to re-open ICAs)

riley-stride · 2024-01-09T23:54:11Z

x/stakeibc/keeper/icacallbacks_undelegate.go

@@ -66,6 +66,11 @@ func (k Keeper) UndelegateCallback(ctx sdk.Context, packet channeltypes.Packet,
 		k.Logger(ctx).Error(utils.LogICACallbackStatusWithHostZone(chainId, ICACallbackID_Undelegate,
 			icacallbackstypes.AckResponseStatus_FAILURE, packet))

+		// Set NativeTokenAmounts on these HZUs to 0
+		if err := k.SetNativeTokensToZeroInUnbondingRecords(ctx, chainId, undelegateCallback.EpochUnbondingRecordIds); err != nil {
+			return err


Should we throw an error here to revert state if downstream UpdateNativeTokensForHostZoneUnbondingRecord fails, or maybe throw an error within UpdateNativeTokensForHostZoneUnbondingRecord if it fails?

if an error is thrown here, I don't think any state will be committed

app/upgrades/v17/upgrades.go

riley-stride · 2024-01-10T00:03:29Z

app/upgrades/v17/upgrades.go

+			// store if the unbonding has not been initiated
+			unbondingNotInitiated := hostZoneUnbonding.Status == recordtypes.HostZoneUnbonding_UNBONDING_QUEUE
+
+			// Loop through User Redemption Records and insert an estimated stTokenAmount


Do we want update records that have the following statuses all in the same way?

HostZoneUnbonding_UNBONDING_QUEUE

HostZoneUnbonding_UNBONDING_IN_PROGRESS <= is there a risk we are going to catch any of these in flight?

HostZoneUnbonding_EXIT_TRANSFER_QUEUE

HostZoneUnbonding_EXIT_TRANSFER_IN_PROGRESS

It's a great question! Thinking through each of these cases:

HostZoneUnbonding_UNBONDING_QUEUE, i.e. tokens have been redeemed by the user but NOT unbonded. I think we do want to convert these (e.g. move all nativeTokenAmounts to stTokenAmounts)

HostZoneUnbonding_UNBONDING_IN_PROGRESS, i.e. there is currently an unbonding in flight. I don't think anything bad will happen here, as all we're doing is adding a variable (e.g. stTokenAmount), but not modifying any existing variables.

HostZoneUnbonding_EXIT_TRANSFER_QUEUE, i.e. unbonding has been initiated, waiting for unbonding to finish. Feels similar to (2) in that we're adding fields (stTokenAmount) but not removing any. I don't think stTokenAmount will be used after the unbonding has been initiated, but please double check me on that!

HostZoneUnbonding_EXIT_TRANSFER_IN_PROGRESS, i.e. unbonding has finished, tokens are being swept. Same as (2) and (3), I think the extra field won't modify anything.

Please double check me though that the extra fields won't mess with anything!

Yep, makes sense.

Thinking about this a bit more, not only will nothing bad happen here, in fact we need to migrate these or their undelegate callbacks will fail because in UndelegateCallback we invoke SetNativeTokensToZeroInUnbondingRecords which references their stTokenAmount attribute. If they have no stTokenAmount that callback will return err.

Agreed, we should migrate it. And feels safe: I can't find a reference to stTokenAmount in any code post-UndelegateCallback. Imo this is the same as 4 but different than 2 (2 needs the migration to avoid erroring).

Yeah I think we should migrate it. Imo this is the same as 3 but different than 2 (2 needs the migration to avoid erroring).

Ah yeah great call re:(2)! Let's keep this as is then, seems like the migration is good for all of the cases

Agreed let's keep it as is

Leaving this open so @sampocs and @asalzmann can think through each case with this context as well. Feel free to resolve after.

app/upgrades/v17/upgrades.go

sampocs

sorry kinda running out of steam rn, but just dropping this partial review for now and will finish the review first thing in the morning!

sampocs · 2024-01-10T03:06:57Z

x/stakeibc/keeper/unbonding_records.go

+	if hostZoneRecord.Status == recordstypes.HostZoneUnbonding_UNBONDING_QUEUE && hostZoneRecord.StTokenAmount.GT(sdkmath.ZeroInt()) {
+		return true
+	}
+	return false


Suggested change

if hostZoneRecord.Status == recordstypes.HostZoneUnbonding_UNBONDING_QUEUE && hostZoneRecord.StTokenAmount.GT(sdkmath.ZeroInt()) {

return true

}

return false

return hostZoneRecord.Status == recordstypes.HostZoneUnbonding_UNBONDING_QUEUE && hostZoneRecord.StTokenAmount.GT(sdkmath.ZeroInt())

sampocs · 2024-01-10T03:14:12Z

x/stakeibc/keeper/unbonding_records.go

@@ -96,6 +111,74 @@ func (k Keeper) GetTotalUnbondAmountAndRecordsIds(ctx sdk.Context, chainId strin
 	return totalUnbonded, unbondingRecordIds
 }

+// Update Native Amounts in the Host Zone Unbonding Record (and associated User Redemption Records) to reflect the current redemption rate


A couple nit comments in this PR to clean things up a bit and keep things standardized:

Use .Attribute instead of .GetAttribute()

Add some vertical whitespace between logical groups of logic (makes the review much easier! - similar to new line breaks in a paragraph)

We don't have to do that errMsg variable format anymore, and no need to log each error cause their logged upstream, we can just return the wrapped error

sampocs · 2024-01-10T03:18:03Z

x/stakeibc/keeper/unbonding_records.go

+	hostZoneUnbondingRecord.NativeTokenAmount = totalNativeAmount
+	updatedEpochUnbondingRecord, success := k.RecordsKeeper.AddHostZoneToEpochUnbondingRecord(ctx, epochNumber, chainId, hostZoneUnbondingRecord)
+	if !success {
+		// TODO QUESTION: what should we do on failure? do we revert the userRedemptionRecord changes above?


Even if we throw an error, state doesn't get reverted since this is in the begin blocker. We should use a cache context wrapper around this function and throw an error

sampocs · 2024-01-10T03:19:40Z

x/stakeibc/keeper/unbonding_records.go

+			return errorsmod.Wrapf(types.ErrHostZoneNotFound, errMsg)
+		}
+		success := k.UpdateNativeTokensForHostZoneUnbondingRecord(ctx, epochUnbondingRecordId, hostZoneUnbonding, true)
+		if !success {


Mentioned in the other thread as well - state does not get reverted in the begin/end blocker even if we return an error. We should wrap this in a temporary context so we can discard state changes if an error is thrown. I believe this is already implemented in an old PR

sampocs · 2024-01-10T03:27:45Z

x/stakeibc/keeper/unbonding_records.go

@@ -74,6 +74,14 @@ func (k Keeper) CreateEpochUnbondingRecord(ctx sdk.Context, epochNumber uint64)
 	return true
 }

+// Helper function to evaluate if a host zone unbonding record still needs to be initiated
+func (k Keeper) ShouldInitiateHostZoneUnbondingRecord(ctx sdk.Context, hostZoneRecord *recordstypes.HostZoneUnbonding) bool {


thoughts on renaming to ShouldHostZoneRecordUnbond?

Imo the proposed change is more clear.

sampocs · 2024-01-10T03:30:13Z

x/stakeibc/keeper/unbonding_records.go

+				k.Logger(ctx).Error(utils.LogWithHostZone(chainId, "Failed to update native tokens for host zone unbonding record in epoch %d", epochUnbonding.EpochNumber))
+				continue
+			}
+
 			totalUnbonded = totalUnbonded.Add(hostZoneRecord.NativeTokenAmount)


are you sure this summation will work? I think the native amount might always be 0 here

Good eye. Agreed I don't think this will work. @shellvish @sampocs should we run this PR through the dockernet integration tests to make sure the high level flow works? Would be nice to have that guarantee as we review.

Actually, I take that back. hostZoneRecord is passed into UpdateNativeTokensForHostZoneUnbondingRecord and updated here. I think this does work.

we should try and clean this up a bit. It's a bit dangerous to update pointers inside functions and then make assumptions in the calling function

In general, I think we should either:

Return the struct from the updating function, or

Write to state in the updating function, and read that value back in the calling function

I'll take a peak at what makes the most sense here. Unfortunately, the EUR/HZU schema makes each approach a bit messy

sampocs · 2024-01-10T15:09:03Z

Considering our unbonding flow is already pretty tricky, I think we should optimize for readability over performance.

One paradigm that I think helps remove implicit assumptions is having a "main" like function (UnbondFromHost) and then having the sub functions be small/focused and explicit about whether they are just doing calculations or are updating state.

That said, I think we should disentangle this SetNativeAmount functions from GetTotalUnbondAmountAndRecordsIds. So in the main we have something like:

// set the native amount for each EUR
// sum the total native amount

This requires we do another loop, but then we don't have to modify the existing GetTotalUnbondAmountAndRecordsIds function at all, we can keep it focused on the one task, and we don't have to rely on pointers/global side effects.

If you're all in agreement with this, I can open a PR to this one

…nbond

asalzmann

didn't have time to review the migration logic yet (but planning on doing that last, since it's more straightforward than implementing the functions themselves)

Do we handle failure cases (e.g. callback errors or times out) correctly?

I think so

Do we migrate our current data structures over properly?

Haven't checked yet

Does it ever break any assumptions if the NativeTokenAmount on a HostZone is set to 0 before unbondings finish

Can you clarify this question? Do you mean HostZoneUnbonding?

If so, I think we clear out EURs when all HZUs have native_token_amount == 0

asalzmann · 2024-01-10T18:57:09Z

proto/stride/records/records.proto

@@ -18,6 +18,10 @@ message UserRedemptionRecord {
  uint64 epoch_number = 7;
  bool claim_is_pending = 8;
  reserved 2;
+  string st_token_amount = 9 [


nit: add comment explaining what values to expect for st_token_amount and amount

roughly

st_token_amount is set upon unbonding

amount is set to 0 when the URR is created, and set to an amount to be unbonded when the unbonding is triggered. if it's in status UNBONDING_QUEUE, it should be 0, if it's in any other status - UNBONDING_IN_PROGRESS or later - it should be set to an amount

asalzmann · 2024-01-10T18:58:36Z

x/stakeibc/keeper/icacallbacks_undelegate.go

@@ -66,6 +66,11 @@ func (k Keeper) UndelegateCallback(ctx sdk.Context, packet channeltypes.Packet,
 		k.Logger(ctx).Error(utils.LogICACallbackStatusWithHostZone(chainId, ICACallbackID_Undelegate,
 			icacallbackstypes.AckResponseStatus_FAILURE, packet))

+		// Set NativeTokenAmounts on these HZUs to 0


nit: add a comment above in line 56 ("// No need to reset the unbonding record status...") that explains we also don't need to reset the amount to 0 on timeout, because it's set when the ICA channel is re-opened

asalzmann · 2024-01-10T18:59:46Z

x/stakeibc/keeper/icacallbacks_undelegate.go

@@ -66,6 +66,11 @@ func (k Keeper) UndelegateCallback(ctx sdk.Context, packet channeltypes.Packet,
 		k.Logger(ctx).Error(utils.LogICACallbackStatusWithHostZone(chainId, ICACallbackID_Undelegate,
 			icacallbackstypes.AckResponseStatus_FAILURE, packet))

+		// Set NativeTokenAmounts on these HZUs to 0
+		if err := k.SetNativeTokensToZeroInUnbondingRecords(ctx, chainId, undelegateCallback.EpochUnbondingRecordIds); err != nil {
+			return err


if an error is thrown here, I don't think any state will be committed

asalzmann · 2024-01-10T19:00:53Z

x/stakeibc/keeper/msg_server_restore_interchain_account.go

 		// Revert UNBONDING_IN_PROGRESS records to UNBONDING_QUEUE
-		err := k.RecordsKeeper.SetHostZoneUnbondings(ctx, hostZone.ChainId, epochNumberForPendingUnbondingRecords, recordtypes.HostZoneUnbonding_UNBONDING_QUEUE)
+		err = k.RecordsKeeper.SetHostZoneUnbondings(ctx, hostZone.ChainId, epochNumberForPendingUnbondingRecords, recordtypes.HostZoneUnbonding_UNBONDING_QUEUE)


Should we rename SetHostZoneUnbondings -> SetHostZoneUnbondingsStatus for clarity?

Also, would it make sense to consolidate SetNativeTokensToZeroInUnbondingRecords and SetHostZoneUnbondings?

asalzmann · 2024-01-10T19:03:26Z

x/stakeibc/keeper/msg_server_restore_interchain_account.go

+		if err != nil {
+			errMsg := fmt.Sprintf("unable to set native token amounts to 0 for chainId: %s and epochUnbondingRecordIds: %v, err: %s")
+			k.Logger(ctx).Error(errMsg)
+			// TODO Question: should we return an error here, or just proceed?


imo we should return an error - we don't want to re-open the ICA channel if we're in a partially recovered state.

asalzmann · 2024-01-10T19:21:29Z

x/stakeibc/keeper/unbonding_records.go

+		}
+		// update the amount, based on the current redemption rate (or set to zero if desired)
+		nativeAmount := sdkmath.ZeroInt()
+		if !setNativeAmountsToZero {


imo we shouldn't mix this into the function - we should have

UpdateNativeTokensForHostZoneUnbondingRecord ResetNativeTokensForHostZoneUnbondingRecord

and these could optionally call a sub function

the current setup of SetNativeTokensToZeroInUnbondingRecords calling UpdateNativeTokensForHostZoneUnbondingRecord is confusing

asalzmann · 2024-01-10T19:22:24Z

x/stakeibc/keeper/unbonding_records.go

+	hostZoneUnbondingRecord.NativeTokenAmount = totalNativeAmount
+	updatedEpochUnbondingRecord, success := k.RecordsKeeper.AddHostZoneToEpochUnbondingRecord(ctx, epochNumber, chainId, hostZoneUnbondingRecord)
+	if !success {
+		// TODO QUESTION: what should we do on failure? do we revert the userRedemptionRecord changes above?


it's not necessarily in the begin blocker - re-opening ICAs is just a tx

asalzmann · 2024-01-10T19:24:49Z

x/stakeibc/keeper/unbonding_records.go

+			return errorsmod.Wrapf(types.ErrHostZoneNotFound, errMsg)
+		}
+		success := k.UpdateNativeTokensForHostZoneUnbondingRecord(ctx, epochUnbondingRecordId, hostZoneUnbonding, true)
+		if !success {


same comment as above - this is called outside of the begin blocker (acks, tx to re-open ICAs)

sampocs · 2024-01-11T02:41:49Z

closing in favor of #1053

shellvish added 4 commits January 9, 2024 10:41

update user redemption rates dynamically

9de0cc1

make NativeAmount 0 until unbonding succeeds

a0d29d9

Add UpgradeHandler Logic

9c45554

handle failure cases for dynamic RRs

a71b9b4

shellvish requested review from asalzmann, riley-stride and sampocs January 9, 2024 23:02

bugfix

3e84687

riley-stride reviewed Jan 10, 2024

View reviewed changes

mergify bot mentioned this pull request Jan 10, 2024

Rename "amount" to "nativeTokenAmount" in UserRedemptionRecord #1050

Closed

address Riley's PR comments

69a9b7b

sampocs reviewed Jan 10, 2024

View reviewed changes

rename ShouldInitiateHostZoneUnbondingRecord to ShouldHostZoneRecordU…

01da18c

…nbond

asalzmann reviewed Jan 10, 2024

View reviewed changes

sampocs closed this Jan 11, 2024

sampocs mentioned this pull request Jan 11, 2024

dynamically update user redemption records during unbonding #1053

Merged

Updates User Redemption Records dynamically before unbonding #1049

Updates User Redemption Records dynamically before unbonding #1049

Conversation

shellvish commented Jan 9, 2024 • edited Loading

riley-stride left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riley-stride Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shellvish Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

riley-stride Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampocs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampocs Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

sampocs commented Jan 10, 2024 • edited Loading

asalzmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampocs commented Jan 11, 2024

shellvish commented Jan 9, 2024 •

edited

Loading

riley-stride Jan 10, 2024 •

edited

Loading

shellvish Jan 10, 2024 •

edited

Loading

riley-stride Jan 10, 2024 •

edited

Loading

sampocs Jan 10, 2024 •

edited

Loading

sampocs commented Jan 10, 2024 •

edited

Loading