fix(bank): fix unhandled error for vesting #13690

fedekunze · 2022-10-29T10:38:43Z

Description

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)

codecov · 2022-10-29T11:01:21Z

Codecov Report

Merging #13690 (e0dae11) into main (155bcfa) will decrease coverage by 0.59%.
The diff coverage is 91.66%.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13690      +/-   ##
==========================================
- Coverage   56.03%   55.43%   -0.60%     
==========================================
  Files         646      646              
  Lines       55784    55790       +6     
==========================================
- Hits        31258    30928     -330     
- Misses      21984    22344     +360     
+ Partials     2542     2518      -24

Impacted Files	Coverage Δ
x/bank/keeper/send.go	`80.79% <90.90%> (+0.38%)`	⬆️
types/coin.go	`94.51% <100.00%> (ø)`
x/distribution/client/cli/query.go	`0.00% <0.00%> (-73.95%)`	⬇️
x/distribution/client/cli/tx.go	`5.49% <0.00%> (-69.79%)`	⬇️
crypto/keys/internal/ecdsa/privkey.go	`83.01% <0.00%> (+1.88%)`	⬆️

julienrbrt

lgtm! maybe we should add a test, though.

CHANGELOG.md

alexanderbez · 2022-10-31T00:15:17Z

x/bank/keeper/send.go

+		// NOTE: denom and amount validation should occur before transfer
+		locked := sdk.Coin{
+			Denom:  coin.Denom,
+			Amount: lockedCoins.AmountOfNoDenomValidation(coin.Denom),


What benefit do we have to calling AmountOfNoDenomValidation here over AmountOf? Since validation should occur prior as you pointed out, AmountOf seems practical, no?

NewCoin and AmountOf validate the denom. Here we don't need to because we are already passing a Coins type as an argument. By using AmountOfNoDenomValidation and sdk.Coin{} we're saving two unnecessary denom validations just in this operation

I'm a bit against this. The idea is that these APIs exist for a reason, and unless explicitly not needed, e.g. denom validation is explicitly not required, then New* constructors should always be called. This also keeps consistency with the rest of the codebase and makes the PR diff smaller.

I tend to agree on this one, I really like consistency. Unless there's a noticeable performance difference I would go with AmountOf.

Shall we just push to this branch and fix it then?

x/bank/keeper/send.go

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

facundomedica

LGTM, as Julien said would be nice to have a small test for this

likhita-809

lgtm

fedekunze · 2022-11-01T17:20:24Z

thanks for the reviews! Unfortunately, I don't have time to work on tests until the end of the week. I might ask someone from Evmos to add them on my behalf

…or (#13733)

julienrbrt · 2022-11-02T13:21:04Z

Thanks, @MalteHerrmann for the test!

## Description Closes: #13691 --- ### 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/main/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/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/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 3034a9d) # Conflicts: # CHANGELOG.md # x/bank/keeper/keeper_test.go

## Description Closes: #13691 --- ### 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/main/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/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/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 3034a9d) # Conflicts: # CHANGELOG.md # types/coin.go # x/bank/keeper/keeper_test.go # x/bank/keeper/send.go

fix(bank): fix unhanlded error for vesting

0d7c57e

github-actions bot added the C:x/bank label Oct 29, 2022

fedekunze added 2 commits October 29, 2022 12:41

fix conflicts

f5f8a40

changelog

2926c1e

fedekunze marked this pull request as ready for review October 29, 2022 10:49

fedekunze requested a review from a team as a code owner October 29, 2022 10:49

fedekunze requested review from tac0turtle and alexanderbez October 29, 2022 10:50

fedekunze added backport/0.45.x backport/0.46.x PR scheduled for inclusion in the v0.46's next stable release T:Bug labels Oct 29, 2022

better error msg

d8f3d85

julienrbrt approved these changes Oct 29, 2022

View reviewed changes

Merge branch 'main' into fedekunze/handle-locked-amt

48d622a

alexanderbez reviewed Oct 31, 2022

View reviewed changes

fedekunze and others added 2 commits October 31, 2022 08:02

Apply suggestions from code review

0a696b0

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Merge branch 'main' into fedekunze/handle-locked-amt

3af9fd6

tac0turtle assigned alexanderbez, amaury1093 and facundomedica Oct 31, 2022

facundomedica approved these changes Oct 31, 2022

View reviewed changes

likhita-809 approved these changes Nov 1, 2022

View reviewed changes

atheeshp and others added 3 commits November 2, 2022 10:18

Merge branch 'main' into fedekunze/handle-locked-amt

2d910ec

tests: Add test to verify trying to send locked tokens raises an err…

f944e06

…or (#13733)

Merge branch 'main' into fedekunze/handle-locked-amt

8314b0a

likhita-809 and others added 2 commits November 3, 2022 11:14

Merge branch 'main' into fedekunze/handle-locked-amt

f445888

re-add denom validation

9a93321

julienrbrt requested a review from alexanderbez November 3, 2022 16:19

julienrbrt added 2 commits November 3, 2022 17:25

Merge branch 'main' into fedekunze/handle-locked-amt

e0dae11

Merge branch 'main' into fedekunze/handle-locked-amt

3c075a9

julienrbrt added the A:automerge Automatically merge PR once all prerequisites pass. label Nov 3, 2022

mergify bot merged commit 3034a9d into main Nov 3, 2022

mergify bot deleted the fedekunze/handle-locked-amt branch November 3, 2022 18:22

mergify bot mentioned this pull request Nov 3, 2022

fix(bank): fix unhandled error for vesting (backport #13690) #13750

Closed

mergify bot mentioned this pull request Nov 3, 2022

fix(bank): fix unhandled error for vesting (backport #13690) #13751

Closed

julienrbrt mentioned this pull request Nov 3, 2022

chore: add TestSendCoins_Invalid_SendLockedCoins to intregration/e2e #13752

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(bank): fix unhandled error for vesting #13690

fix(bank): fix unhandled error for vesting #13690

fedekunze commented Oct 29, 2022 •

edited

Loading

codecov bot commented Oct 29, 2022 •

edited

Loading

julienrbrt left a comment

alexanderbez Oct 31, 2022

fedekunze Oct 31, 2022 •

edited

Loading

alexanderbez Nov 1, 2022

facundomedica Nov 2, 2022

julienrbrt Nov 3, 2022

alexanderbez Nov 3, 2022

julienrbrt Nov 3, 2022

facundomedica left a comment

likhita-809 left a comment

fedekunze commented Nov 1, 2022

julienrbrt commented Nov 2, 2022

fix(bank): fix unhandled error for vesting #13690

fix(bank): fix unhandled error for vesting #13690

Conversation

fedekunze commented Oct 29, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Oct 29, 2022 • edited Loading

Codecov Report

julienrbrt left a comment

Choose a reason for hiding this comment

alexanderbez Oct 31, 2022

Choose a reason for hiding this comment

fedekunze Oct 31, 2022 • edited Loading

Choose a reason for hiding this comment

alexanderbez Nov 1, 2022

Choose a reason for hiding this comment

facundomedica Nov 2, 2022

Choose a reason for hiding this comment

julienrbrt Nov 3, 2022

Choose a reason for hiding this comment

alexanderbez Nov 3, 2022

Choose a reason for hiding this comment

julienrbrt Nov 3, 2022

Choose a reason for hiding this comment

facundomedica left a comment

Choose a reason for hiding this comment

likhita-809 left a comment

Choose a reason for hiding this comment

fedekunze commented Nov 1, 2022

julienrbrt commented Nov 2, 2022

fedekunze commented Oct 29, 2022 •

edited

Loading

codecov bot commented Oct 29, 2022 •

edited

Loading

fedekunze Oct 31, 2022 •

edited

Loading