fix: fix the cancel unbond by gsk967 · Pull Request #12967 · cosmos/cosmos-sdk

gsk967 · 2022-08-19T07:25:57Z

Description

This pull request contains the fix regarding cancel unbonding delegations

It will remove duplicate ubdEntries which have same creation_height and create new ubdEntry with updated balance and initial_balance

Closes: #12931

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

test.go

x/staking/migrations/v5/store.go

codecov · 2022-08-19T08:23:34Z

Codecov Report

Merging #12967 (c6654a8) into main (07c549b) will decrease coverage by 0.15%.
The diff coverage is 62.85%.

❗ Current head c6654a8 differs from pull request most recent head eb38e0e. Consider uploading reports for the commit eb38e0e to get more accurate results

@@            Coverage Diff             @@
##             main   #12967      +/-   ##
==========================================
- Coverage   55.84%   55.68%   -0.16%     
==========================================
  Files         649      644       -5     
  Lines       55022    54935      -87     
==========================================
- Hits        30726    30591     -135     
- Misses      21822    21878      +56     
+ Partials     2474     2466       -8

Impacted Files	Coverage Δ
x/staking/types/delegation.go	`55.50% <0.00%> (-4.61%)`	⬇️
x/staking/migrations/v4/store.go	`82.81% <84.61%> (+7.81%)`	⬆️
x/genutil/gentx.go	`0.00% <0.00%> (-83.88%)`	⬇️
x/genutil/module.go	`9.09% <0.00%> (-9.10%)`	⬇️
tx/textual/valuerenderer/int.go
tx/textual/valuerenderer/bytes.go
tx/textual/valuerenderer/dec.go
tx/textual/valuerenderer/timestamp.go
tx/textual/valuerenderer/valuerenderer.go

x/staking/keeper/delegation.go

atheeshp

looks good, except few doubts.

CHANGELOG.md

x/staking/keeper/delegation.go

x/staking/migrations/v5/store.go

x/staking/types/delegation.go

CHANGELOG.md

x/staking/migrations/v5/store.go

x/staking/types/delegation.go

likhita-809

lgtm

atheeshp

@gsk967 can you recheck the logic at store migration

x/staking/migrations/v5/store.go

amaury1093

I like the solution, good work. Will make a more thorough review tomorrow.

x/staking/module.go

amaury1093 · 2022-08-22T16:25:14Z

x/staking/types/delegation.go

@@ -130,8 +130,27 @@ func NewUnbondingDelegation(

 // AddEntry - append entry to the unbonding delegation
 func (ubd *UnbondingDelegation) AddEntry(creationHeight int64, minTime time.Time, balance math.Int) {


let's add a unit test for this function

amaury1093

Logic looks good to me, but let's add a unit test for AddEntry before merging.

A nice to have would be a migration test, but not blocking.

x/staking/migrations/v4/store.go

amaury1093 · 2022-08-23T11:26:58Z

x/staking/types/delegation.go

@@ -130,8 +130,27 @@ func NewUnbondingDelegation(

 // AddEntry - append entry to the unbonding delegation


Let's update this godoc too

x/staking/types/delegation.go

gsk967 · 2022-08-24T06:49:06Z

@AmauryM I added store migrations and tests, can you please look into those changes.

atheeshp

lgtm, few nits(mostly spell checks) & Amaury's comments.

x/staking/keeper/delegation_test.go

x/staking/migrations/v4/migrations_test.go

x/staking/migrations/v4/store.go

amaury1093

LGTM, just pending latest naming nits

x/staking/migrations/v4/migrations_test.go

fix: fix the cancel-unbond issue

f560e44

github-actions bot added the C:x/staking label Aug 19, 2022

gsk967 added 2 commits August 19, 2022 13:06

Merge branch 'main' into sai/fix_12931

3d7a8f1

updated the changelog

9d2f122

gsk967 marked this pull request as ready for review August 19, 2022 07:43

gsk967 requested a review from a team August 19, 2022 07:43

chroe: removed un wanted code

fd7affc

github-advanced-security bot found potential problems Aug 19, 2022

View reviewed changes

test.go Fixed Show fixed Hide fixed

x/staking/migrations/v5/store.go Fixed Show fixed Hide fixed

chore: removed max ubd entries check

9393857

github-actions bot added the C:Simulations label Aug 19, 2022

atheeshp reviewed Aug 19, 2022

View reviewed changes

x/staking/keeper/delegation.go Show resolved Hide resolved

chore: add max entries check for ubd

0c3abf9

gsk967 requested a review from atheeshp August 19, 2022 10:06

atheeshp reviewed Aug 19, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

x/staking/keeper/delegation.go Show resolved Hide resolved

x/staking/migrations/v5/store.go Outdated Show resolved Hide resolved

x/staking/types/delegation.go Show resolved Hide resolved

chore: address the pr comments

fbbf1ef

github-actions bot removed the C:Simulations label Aug 19, 2022

alexanderbez reviewed Aug 19, 2022

View reviewed changes

gsk967 and others added 3 commits August 20, 2022 10:12

Merge branch 'main' into sai/fix_12931

3802ec4

chore: address the pr comments

eef238b

Merge branch 'main' into sai/fix_12931

f1e6b20

alexanderbez reviewed Aug 20, 2022

View reviewed changes

x/staking/migrations/v5/store.go Outdated Show resolved Hide resolved

alexanderbez reviewed Aug 20, 2022

View reviewed changes

x/staking/types/delegation.go Outdated Show resolved Hide resolved

x/staking/types/delegation.go Show resolved Hide resolved

gsk967 and others added 2 commits August 22, 2022 09:51

Merge branch 'main' into sai/fix_12931

61b1d1c

chore: address the pr comments++

d9ff332

alexanderbez approved these changes Aug 22, 2022

View reviewed changes

alexanderbez added 2 commits August 22, 2022 10:02

Merge branch 'main' into sai/fix_12931

0ab51d6

Merge branch 'main' into sai/fix_12931

1a008ba

likhita-809 approved these changes Aug 22, 2022

View reviewed changes

atheeshp reviewed Aug 22, 2022

View reviewed changes

x/staking/migrations/v5/store.go Outdated Show resolved Hide resolved

x/staking/migrations/v5/store.go Outdated Show resolved Hide resolved

x/staking/migrations/v5/store.go Outdated Show resolved Hide resolved

x/staking/migrations/v5/store.go Outdated Show resolved Hide resolved

atheeshp reviewed Aug 22, 2022

View reviewed changes

x/staking/migrations/v5/store.go Outdated Show resolved Hide resolved

amaury1093 reviewed Aug 22, 2022

View reviewed changes

gsk967 and others added 2 commits August 23, 2022 10:26

Merge branch 'main' into sai/fix_12931

505b263

chore: address the pr comments ++

184bfd8

amaury1093 reviewed Aug 23, 2022

View reviewed changes

gsk967 and others added 3 commits August 24, 2022 09:31

Merge branch 'main' into sai/fix_12931

6986d14

test: add store migrations

1cc4806

chore: small fix

62498a3

gsk967 requested review from amaury1093 and atheeshp and removed request for amaury1093 and atheeshp August 24, 2022 06:49

atheeshp approved these changes Aug 24, 2022

View reviewed changes

amaury1093 approved these changes Aug 24, 2022

View reviewed changes

x/staking/migrations/v4/migrations_test.go Outdated Show resolved Hide resolved

chore: address the review comments++

c6654a8

amaury1093 added the A:automerge label Aug 24, 2022

Merge branch 'main' into sai/fix_12931

eb38e0e

amaury1093 added backport/0.46.x and removed backport/0.46.x labels Aug 24, 2022

mergify bot merged commit 81028cf into cosmos:main Aug 24, 2022

This was referenced May 5, 2023

[Bug]: AfterUnbondingInitiated can be called with invalid unbondingID #16042

Closed

fix: call AfterUnbondingInitiated for new unbonding entries only #16043

Merged

MSalopek mentioned this pull request Jun 21, 2023

refactor PutUnbondingOnHold handling cosmos/interchain-security#1045

Closed

MSalopek mentioned this pull request Aug 3, 2023

support cosmos-sdk with LSM changes in ICS v2 cosmos/interchain-security#1183

Closed

5 tasks

mpoke mentioned this pull request Aug 4, 2023

feat: add LSM to the SDK's v0.45.16-ics-lsm branch #16747

Merged

19 tasks

		@@ -130,8 +130,27 @@ func NewUnbondingDelegation(

		// AddEntry - append entry to the unbonding delegation
		func (ubd *UnbondingDelegation) AddEntry(creationHeight int64, minTime time.Time, balance math.Int) {

Conversation

gsk967 commented Aug 19, 2022

Description

Author Checklist

Reviewers Checklist

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

atheeshp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

likhita-809 left a comment

Choose a reason for hiding this comment

Uh oh!

atheeshp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amaury1093 Aug 22, 2022

Choose a reason for hiding this comment

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amaury1093 Aug 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gsk967 commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atheeshp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amaury1093 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecov bot commented Aug 19, 2022 •

edited

Loading

gsk967 commented Aug 24, 2022 •

edited

Loading

atheeshp left a comment •

edited

Loading