-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
remove resulting value return from AddCoins and SubtractCoins #7327
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7327 +/- ##
==========================================
- Coverage 56.76% 56.76% -0.01%
==========================================
Files 585 585
Lines 40678 40674 -4
==========================================
- Hits 23092 23088 -4
Misses 15645 15645
Partials 1941 1941 |
Any idea what's with the codecov on this PR? It seems to have warnings on dozens of files that weren't actually changed, hard to review. |
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.
utACK
Seems like a new Github feature; at least the files are separated, ignoring the warnings for now. |
Description
While testing
ibc-transfer
I noticed function like minting and sending in bank allow for successful execution with empty coin sets as a parameter. While this shouldn't cause any issues from what I can tell,AddCoins
andSubtractCoins
will act semantically different then intended when receiving empty coin sets as parameters.The
resultingValue
returned in these functions will be an empty coin set, when the provided argument is also an empty coin set. If any implementation relies on these functions to return a resulting value that is the value at the address after being added or subtracted to then they will likely have a bug in their code.I have removed this value for now which forces code to do a separate query to
GetBalance
instead of relying on the returned value. We should come to consensus on if empty coin sets should be disallowed as valid parameters to bank functions. I have written up some more thoughts on #7046closes: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes