Skip to content
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

feat: add migrations for balances with zero coins #9664

Merged
merged 9 commits into from
Jul 13, 2021

Conversation

atheeshp
Copy link
Contributor

@atheeshp atheeshp commented Jul 9, 2021

Description

Closes: #9653


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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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)

@atheeshp atheeshp requested a review from amaury1093 July 9, 2021 12:26
@github-actions github-actions bot added C:x/bank C:x/genutil genutil module issues labels Jul 9, 2021
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! Thanks @atheeshp, I left a couple of aesthetic comments.

I'm going to try this manually

x/genutil/migrations/v043/migrate.go Show resolved Hide resolved
x/bank/migrations/v043/store.go Show resolved Hide resolved
x/bank/migrations/v043/json_test.go Outdated Show resolved Hide resolved
x/bank/migrations/v043/json.go Show resolved Hide resolved
x/bank/migrations/v043/json.go Outdated Show resolved Hide resolved
@atheeshp atheeshp marked this pull request as ready for review July 9, 2021 16:36
@atheeshp atheeshp requested a review from amaury1093 July 9, 2021 16:36
@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #9664 (52136fe) into master (0027111) will increase coverage by 27.93%.
The diff coverage is 63.61%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #9664       +/-   ##
===========================================
+ Coverage   35.48%   63.41%   +27.93%     
===========================================
  Files         332      573      +241     
  Lines       32620    37597     +4977     
===========================================
+ Hits        11575    23844    +12269     
+ Misses      19819    11891     -7928     
- Partials     1226     1862      +636     
Impacted Files Coverage Δ
client/keys/types.go 100.00% <ø> (+100.00%) ⬆️
client/query.go 16.98% <ø> (ø)
client/rpc/block.go 0.00% <ø> (ø)
client/rpc/status.go 67.74% <ø> (ø)
client/rpc/validators.go 0.00% <ø> (ø)
client/test_helpers.go 0.00% <ø> (ø)
client/tx/factory.go 27.00% <ø> (ø)
client/tx/legacy.go 68.42% <ø> (ø)
client/tx/tx.go 40.83% <ø> (ø)
client/utils.go 41.93% <ø> (-41.40%) ⬇️
... and 683 more

Comment on lines +14 to +15
b.Coins = sdk.NewCoins(b.Coins...) // prunes zero denom.
balances = append(balances, b)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why modify b and not just balances = append(balances, sdk.NewCoins(b.Coins...))?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why modify b and not just balances = append(balances, sdk.NewCoins(b.Coins...))?

if we do this, we may miss b.Address.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow. We don't use b.Address. Are oldBalances being mutated?

Copy link
Contributor Author

@atheeshp atheeshp Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are oldBalances being mutated?

yes, we need to remove some zero denoms from balance of an address (occures when few denoms set to zero but not all)

ex:

{
	"address": "cosmos1fl48vsnmsdzcv85q5d2q4z5ajdha8yu34mf0eh",
	"coins": [
		{
			"amount": "10",
			"denom": "foo"
		},
		{
			"amount": "20",
			"denom": "bar"
		},
		{
			"amount": "0",
			"denom": "foobar"
		}
	]
}

we have to remove foobar here.

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there's a small logic error inside pruneZeroBalances

x/bank/migrations/v043/store.go Outdated Show resolved Hide resolved
x/bank/migrations/v043/store.go Outdated Show resolved Hide resolved
x/bank/migrations/v043/store_test.go Show resolved Hide resolved
@atheeshp atheeshp requested a review from amaury1093 July 13, 2021 07:50
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK, thanks @atheeshp!

TEST:
Send A->B->A on 0.42, so that B has a balance entry with 0 coins. Run migration, make sure B has no balance entry.

@amaury1093 amaury1093 added A:automerge Automatically merge PR once all prerequisites pass. backport/0.43.x labels Jul 13, 2021
@mergify mergify bot merged commit d56c8cd into master Jul 13, 2021
@mergify mergify bot deleted the atheesh/prune-zero-coin-migrations branch July 13, 2021 09:22
mergify bot pushed a commit that referenced this pull request Jul 13, 2021
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

Closes: #9653

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### 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...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)

(cherry picked from commit d56c8cd)

# Conflicts:
#	x/bank/legacy/v043/json.go
#	x/bank/legacy/v043/json_test.go
#	x/bank/legacy/v043/keys.go
#	x/bank/legacy/v043/store_test.go
amaury1093 pushed a commit that referenced this pull request Jul 13, 2021
…9687)

* feat: add migrations for balances with zero coins (#9664)

<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

Closes: #9653

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### 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...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)

(cherry picked from commit d56c8cd)

# Conflicts:
#	x/bank/legacy/v043/json.go
#	x/bank/legacy/v043/json_test.go
#	x/bank/legacy/v043/keys.go
#	x/bank/legacy/v043/store_test.go

* fix conflicts

* fix tests

Co-authored-by: atheeshp <59333759+atheeshp@users.noreply.github.com>
Co-authored-by: atheesh <atheesh@vitwit.com>
JeancarloBarrios pushed a commit to agoric-labs/cosmos-sdk that referenced this pull request Sep 28, 2024
…) (cosmos#9687)

* feat: add migrations for balances with zero coins (cosmos#9664)

<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

Closes: cosmos#9653

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### 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...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)

(cherry picked from commit d56c8cd)

# Conflicts:
#	x/bank/legacy/v043/json.go
#	x/bank/legacy/v043/json_test.go
#	x/bank/legacy/v043/keys.go
#	x/bank/legacy/v043/store_test.go

* fix conflicts

* fix tests

Co-authored-by: atheeshp <59333759+atheeshp@users.noreply.github.com>
Co-authored-by: atheesh <atheesh@vitwit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:x/bank C:x/genutil genutil module issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add migrations for balances with zero coins
3 participants