fix(x/accounts/default/lockup) Lockup account track undelegation when unbonding entry is mature #22254
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,7 @@ func (s *E2ETestSuite) TestPeriodicLockingAccount() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ctx := sdk.NewContext(app.CommitMultiStore(), false, app.Logger()).WithHeaderInfo(header.Info{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Time: currentTime, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| s.setupStakingParams(ctx, app) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ownerAddrStr, err := app.AuthKeeper.AddressCodec().BytesToString(accOwner) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| s.fundAccount(app, ctx, accOwner, sdk.Coins{sdk.NewCoin("stake", math.NewInt(1000000))}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -185,10 +186,11 @@ func (s *E2ETestSuite) TestPeriodicLockingAccount() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| require.Equal(t, len(ubd.Entries), 1) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // check if an entry is added | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| unbondingEntriesResponse := s.queryUnbondingEntries(ctx, app, accountAddr) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| entries := unbondingEntriesResponse.UnbondingEntries | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| require.True(t, entries[0].Amount.Amount.Equal(math.NewInt(100))) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| require.True(t, entries[0].ValidatorAddress == val.OperatorAddress) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Ensure unbonding entries are not empty before accessing Accessing Apply this diff to add a length check: + require.True(t, len(entries) > 0)
require.True(t, entries[0].Amount.Amount.Equal(math.NewInt(100)))
require.True(t, entries[0].ValidatorAddress == val.OperatorAddress)📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Update context time | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -197,6 +199,20 @@ func (s *E2ETestSuite) TestPeriodicLockingAccount() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Time: currentTime.Add(time.Minute * 3), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| t.Run("ok - execute tracking unbonding entry", func(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| msg := &types.MsgTrackUndelegation{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Sender: ownerAddrStr, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Id: 0, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| err = s.executeTx(ctx, msg, app, accountAddr, accOwner) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // check if tracking is updated accordingly | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lockupAccountInfoResponse := s.queryLockupAccInfo(ctx, app, accountAddr) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| delLocking := lockupAccountInfoResponse.DelegatedLocking | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| require.True(t, delLocking.AmountOf("stake").Equal(math.ZeroInt())) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Avoid hardcoding unbonding entry ID; obtain it dynamically Currently, the unbonding entry Apply this diff to obtain the entry ID dynamically: t.Run("ok - execute tracking unbonding entry", func(t *testing.T) {
+ // Retrieve the unbonding entries to get the correct ID
+ unbondingEntriesResponse := s.queryUnbondingEntries(ctx, app, accountAddr)
+ entries := unbondingEntriesResponse.UnbondingEntries
+ require.True(t, len(entries) > 0)
+ entryID := entries[0].Id
msg := &types.MsgTrackUndelegation{
Sender: ownerAddrStr,
- Id: 0,
+ Id: entryID,
}
err = s.executeTx(ctx, msg, app, accountAddr, accOwner)
require.NoError(t, err)
// check if tracking is updated accordingly
lockupAccountInfoResponse := s.queryLockupAccInfo(ctx, app, accountAddr)
delLocking := lockupAccountInfoResponse.DelegatedLocking
require.True(t, delLocking.AmountOf("stake").Equal(math.ZeroInt()))
})📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| t.Run("ok - execute delegate message", func(t *testing.T) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| msg := &types.MsgDelegate{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Sender: ownerAddrStr, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,7 @@ func (s *E2ETestSuite) TestPermanentLockingAccount() { | |||||||||||||||||||||||||||||||
| ctx := sdk.NewContext(app.CommitMultiStore(), false, app.Logger()).WithHeaderInfo(header.Info{ | ||||||||||||||||||||||||||||||||
| Time: currentTime, | ||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||
| s.setupStakingParams(ctx, app) | ||||||||||||||||||||||||||||||||
| ownerAddrStr, err := app.AuthKeeper.AddressCodec().BytesToString(accOwner) | ||||||||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||||||||
| s.fundAccount(app, ctx, accOwner, sdk.Coins{sdk.NewCoin("stake", math.NewInt(1000000))}) | ||||||||||||||||||||||||||||||||
|
|
@@ -109,10 +110,11 @@ func (s *E2ETestSuite) TestPermanentLockingAccount() { | |||||||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||||||||
| require.Equal(t, len(ubd.Entries), 1) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // check if an entry is added | ||||||||||||||||||||||||||||||||
| unbondingEntriesResponse := s.queryUnbondingEntries(ctx, app, accountAddr) | ||||||||||||||||||||||||||||||||
| entries := unbondingEntriesResponse.UnbondingEntries | ||||||||||||||||||||||||||||||||
| require.True(t, entries[0].Amount.Amount.Equal(math.NewInt(100))) | ||||||||||||||||||||||||||||||||
| require.True(t, entries[0].ValidatorAddress == val.OperatorAddress) | ||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. 🛠️ Refactor suggestion Use Using Apply this diff: - require.True(t, entries[0].ValidatorAddress == val.OperatorAddress)
+ require.Equal(t, val.OperatorAddress, entries[0].ValidatorAddress)📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Check for empty unbonding entries before accessing Accessing Apply this diff to add the length check: + require.Greater(t, len(entries), 0)
require.True(t, entries[0].Amount.Amount.Equal(math.NewInt(100)))
require.Equal(t, val.OperatorAddress, entries[0].ValidatorAddress)📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| s.fundAccount(app, ctx, accountAddr, sdk.Coins{sdk.NewCoin("stake", math.NewInt(1000))}) | ||||||||||||||||||||||||||||||||
|
|
@@ -129,4 +131,23 @@ func (s *E2ETestSuite) TestPermanentLockingAccount() { | |||||||||||||||||||||||||||||||
| balance := app.BankKeeper.GetBalance(ctx, randAcc, "stake") | ||||||||||||||||||||||||||||||||
| require.True(t, balance.Amount.Equal(math.NewInt(100))) | ||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Update context time | ||||||||||||||||||||||||||||||||
| ctx = ctx.WithHeaderInfo(header.Info{ | ||||||||||||||||||||||||||||||||
| Time: currentTime.Add(time.Second * 11), | ||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. 💡 Codebase verification Unbonding Time Discrepancies Detected The unbonding periods across the codebase vary and do not consistently align with the test's assumption of a maximum 10-second unbonding period. This may lead to tests failing if unbonding entries have not matured as expected.
🔗 Analysis chainVerify unbonding period before advancing context time Advancing the context time by 11 seconds assumes that the unbonding period is less than or equal to 10 seconds. Please verify that the unbonding period set in the staking parameters matches this assumption to ensure unbonding entries have matured. Run the following script to confirm the unbonding time: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Retrieve the unbonding time from the staking parameters to verify it aligns with the test's time advancement.
# Command:
ast-grep --lang go --pattern 'UnbondingTime: $time' --json 'app.StakingKeeper.SetParams(ctx, stakingtypes.Params{ $$$ })' | jq '.[].meta.captures.time'
Length of output: 261 Script: #!/bin/bash
# Description: Search for the UnbondingTime parameter in the staking parameters setup to verify its value.
# Using ripgrep to find lines where UnbondingTime is set
rg 'UnbondingTime\s*:\s*\w+' --files-with-matches | xargs rg 'UnbondingTime\s*:\s*(\w+)'
Length of output: 1912 |
||||||||||||||||||||||||||||||||
| t.Run("ok - execute tracking unbonding entry", func(t *testing.T) { | ||||||||||||||||||||||||||||||||
| msg := &types.MsgTrackUndelegation{ | ||||||||||||||||||||||||||||||||
| Sender: ownerAddrStr, | ||||||||||||||||||||||||||||||||
| Id: 0, | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| err = s.executeTx(ctx, msg, app, accountAddr, accOwner) | ||||||||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // check if tracking is updated accordingly | ||||||||||||||||||||||||||||||||
| lockupAccountInfoResponse := s.queryLockupAccInfo(ctx, app, accountAddr) | ||||||||||||||||||||||||||||||||
| delLocking := lockupAccountInfoResponse.DelegatedLocking | ||||||||||||||||||||||||||||||||
| require.True(t, delLocking.AmountOf("stake").Equal(math.ZeroInt())) | ||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. 🛠️ Refactor suggestion Add test cases for multiple unbonding entries Currently, the test tracks undelegation for a single entry with |
||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ package lockup | |
|
|
||
| import ( | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| "github.com/stretchr/testify/suite" | ||
|
|
@@ -70,3 +71,25 @@ func (s *E2ETestSuite) queryLockupAccInfo(ctx sdk.Context, app *simapp.SimApp, a | |
|
|
||
| return lockupAccountInfoResponse | ||
| } | ||
|
|
||
| func (s *E2ETestSuite) queryUnbondingEntries(ctx sdk.Context, app *simapp.SimApp, accAddr []byte) *types.QueryUnbondingEntriesResponse { | ||
| req := &types.QueryUnbondingEntriesRequest{} | ||
| resp, err := s.queryAcc(ctx, req, app, accAddr) | ||
| require.NoError(s.T(), err) | ||
| require.NotNil(s.T(), resp) | ||
|
|
||
| unbondingEntriesResponse, ok := resp.(*types.QueryUnbondingEntriesResponse) | ||
| require.True(s.T(), ok) | ||
|
|
||
| return unbondingEntriesResponse | ||
| } | ||
|
|
||
| func (s *E2ETestSuite) setupStakingParams(ctx sdk.Context, app *simapp.SimApp) { | ||
| params, err := app.StakingKeeper.Params.Get(ctx) | ||
| require.NoError(s.T(), err) | ||
|
|
||
| // update unbonding time | ||
| params.UnbondingTime = time.Duration(time.Second * 10) | ||
| err = app.StakingKeeper.Params.Set(ctx, params) | ||
| require.NoError(s.T(), err) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -200,4 +200,5 @@ func (cva ContinuousLockingAccount) RegisterExecuteHandlers(builder *accountstd. | |
|
|
||
| func (cva ContinuousLockingAccount) RegisterQueryHandlers(builder *accountstd.QueryBuilder) { | ||
| accountstd.RegisterQueryHandler(builder, cva.QueryLockupAccountInfo) | ||
| accountstd.RegisterQueryHandler(builder, cva.QueryUnbondingEntries) | ||
sontrinh16 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -102,48 +102,23 @@ func TestContinousAccountUndelegate(t *testing.T) { | |||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| ubdSeq, err := acc.UnbondingSequence.Peek(sdkCtx) | ||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||
| // sequence should be the previous one | ||||||||||||||||||||||||||
| ubdEntry, err := acc.UnbondEntries.Get(sdkCtx, ubdSeq-1) | ||||||||||||||||||||||||||
|
There was a problem hiding this comment. Prevent potential underflow when accessing unbonding entries When subtracting 1 from Consider adding a check to confirm ubdSeq, err := acc.UnbondingSequence.Peek(sdkCtx)
require.NoError(t, err)
+require.True(t, ubdSeq > 0, "Unbonding sequence must be greater than zero")
// sequence should be the previous one
ubdEntry, err := acc.UnbondEntries.Get(sdkCtx, ubdSeq-1)
require.NoError(t, err)📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||
| require.True(t, ubdEntry.Amount.Amount.Equal(math.NewInt(1))) | ||||||||||||||||||||||||||
| require.True(t, ubdEntry.ValidatorAddress == "val_address") | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| _, err = acc.TrackUndelegationEntry(sdkCtx, &lockuptypes.MsgTrackUndelegation{ | ||||||||||||||||||||||||||
| Sender: "owner", | ||||||||||||||||||||||||||
| Id: ubdSeq - 1, | ||||||||||||||||||||||||||
|
There was a problem hiding this comment. Ensure valid unbonding entry ID before tracking undelegation Similar to the previous issue, when passing Add a check before calling +require.True(t, ubdSeq > 0, "Unbonding sequence must be greater than zero")
_, err = acc.TrackUndelegationEntry(sdkCtx, &lockuptypes.MsgTrackUndelegation{
Sender: "owner",
Id: ubdSeq - 1,
})
require.NoError(t, err)
|
||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| delLocking, err = acc.DelegatedLocking.Get(ctx, "test") | ||||||||||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||||||||||
| require.True(t, delLocking.Equal(math.ZeroInt())) | ||||||||||||||||||||||||||
|
There was a problem hiding this comment. Add timeline validation for undelegation process The test should verify the intermediate state where funds are still locked during the unbonding period. Add assertions to check:
Example: // Check intermediate state
delLocking, err = acc.DelegatedLocking.Get(ctx, "test")
require.NoError(t, err)
require.True(t, delLocking.Equal(math.NewInt(1))) // Should still be locked
// Advance time to maturity
sdkCtx = sdkCtx.WithHeaderInfo(header.Info{
Time: ubdEntry.CompletionTime,
})
// Update undelegation entry after maturity
_, err = acc.UpdateUndelegationEntry(sdkCtx, &lockuptypes.MsgUpdateUndelegationEntry{
Sender: "owner",
Id: ubdSeq - 1,
})
require.NoError(t, err)
// Now verify zero locking amount
delLocking, err = acc.DelegatedLocking.Get(ctx, "test")
require.NoError(t, err)
require.True(t, delLocking.Equal(math.ZeroInt())) |
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| func TestContinousAccountSendCoins(t *testing.T) { | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -149,4 +149,5 @@ func (dva DelayedLockingAccount) RegisterExecuteHandlers(builder *accountstd.Exe | |
|
|
||
| func (dva DelayedLockingAccount) RegisterQueryHandlers(builder *accountstd.QueryBuilder) { | ||
| accountstd.RegisterQueryHandler(builder, dva.QueryVestingAccountInfo) | ||
| accountstd.RegisterQueryHandler(builder, dva.QueryUnbondingEntries) | ||
|
There was a problem hiding this comment. 💡 Codebase verification 🔍 Missing Implementation of The
🔗 Analysis chainLGTM. Verify the implementation of QueryUnbondingEntries. The addition of the Please run the following script to verify the existence and implementation of the If the method is not found or the types are not defined, please implement them to ensure the newly added query handler functions correctly. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the implementation of QueryUnbondingEntries method
# Test: Search for the QueryUnbondingEntries method implementation
ast-grep --lang go --pattern $'func (dva DelayedLockingAccount) QueryUnbondingEntries(ctx context.Context, req *lockuptypes.QueryUnbondingEntriesRequest) (*lockuptypes.QueryUnbondingEntriesResponse, error) {
$$$
}'
# Test: Check if the QueryUnbondingEntriesRequest and QueryUnbondingEntriesResponse types are defined
rg --type go -e 'type QueryUnbondingEntriesRequest struct' -e 'type QueryUnbondingEntriesResponse struct'
Length of output: 693 |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,44 +101,23 @@ func TestDelayedAccountUndelegate(t *testing.T) { | |
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| ubdSeq, err := acc.UnbondingSequence.Peek(sdkCtx) | ||
| require.NoError(t, err) | ||
| // sequence should be the previous one | ||
| ubdEntry, err := acc.UnbondEntries.Get(sdkCtx, ubdSeq-1) | ||
| require.NoError(t, err) | ||
| require.True(t, ubdEntry.Amount.Amount.Equal(math.NewInt(1))) | ||
| require.True(t, ubdEntry.ValidatorAddress == "val_address") | ||
|
|
||
| _, err = acc.TrackUndelegationEntry(sdkCtx, &lockuptypes.MsgTrackUndelegation{ | ||
| Sender: "owner", | ||
| Id: ubdSeq - 1, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| delLocking, err = acc.DelegatedLocking.Get(ctx, "test") | ||
| require.NoError(t, err) | ||
| require.True(t, delLocking.Equal(math.ZeroInt())) | ||
| } | ||
|
|
||
| func TestDelayedAccountSendCoins(t *testing.T) { | ||
|
|
||
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.
Prevent potential index out-of-range panic when accessing
entries[0].Before accessing
entries[0], add a check to ensure that theentriesslice is not empty. This avoids a possible runtime panic if the slice is empty.Apply this diff to add the necessary check:
+ require.NotEmpty(t, entries) require.True(t, entries[0].Amount.Amount.Equal(math.NewInt(100))) require.True(t, entries[0].ValidatorAddress == val.OperatorAddress)📝 Committable suggestion