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

fix: propagate funds validation errors #1649

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

aelesbao
Copy link
Contributor

@aelesbao aelesbao commented Oct 3, 2023

The validation functions on the tx type masquerade the root error message for the Funds validation. Having the original error helps to save time when debugging the cause for a failed tx.

One example is if someone sends multiple funds to a contract execution without sorting the denoms, which is one of the validations in the Coins.Validate method. With the error propagation, the developer can quickly determine why the tx failed.

@aelesbao aelesbao requested a review from alpe as a code owner October 3, 2023 10:36
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #1649 (6b8b45c) into main (e0da419) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1649   +/-   ##
=======================================
  Coverage   56.59%   56.59%           
=======================================
  Files          62       62           
  Lines        8194     8194           
=======================================
  Hits         4637     4637           
  Misses       3120     3120           
  Partials      437      437           
Files Coverage Δ
x/wasm/types/tx.go 51.04% <100.00%> (ø)

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
Better error messages make sense. 👍
The deprecated gov beta1 types were removed this morning. Can you rebase your PR please?

The validation functions on the `tx` type masquerade the root error
message for the `Funds` validation. Having the original error helps to
save time when debugging the cause for a failed tx.

One example is if someone sends multiple funds to a contract execution
without sorting the denoms, which is one of the validations in the
`Coins.Validate` method. With the error propagation, the developer can
quickly determine why the tx failed.
@aelesbao aelesbao force-pushed the aelesbao/fix/propagate-funds-err branch from 3a17028 to 6b8b45c Compare October 4, 2023 09:37
@aelesbao
Copy link
Contributor Author

aelesbao commented Oct 4, 2023

Thanks for the reply, @alpe. I've rebased the branch and updated the pull request description.

Copy link
Contributor

@alpe alpe left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for doing this 💐

@alpe alpe merged commit 3ea8575 into CosmWasm:main Oct 5, 2023
8 checks passed
@aelesbao aelesbao deleted the aelesbao/fix/propagate-funds-err branch October 5, 2023 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants