feat(bank): add MintCoinsToAddress #9620
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9620 +/- ##
==========================================
- Coverage 60.54% 60.54% -0.01%
==========================================
Files 588 588
Lines 37447 37460 +13
==========================================
+ Hits 22674 22679 +5
- Misses 12781 12789 +8
Partials 1992 1992
|
| sdk.NewAttribute(AttributeKeyMinter, minter.String()), | ||
| sdk.NewAttribute(sdk.AttributeKeyAmount, amount.String()), | ||
| sdk.NewAttribute(AttributeKeyReceiver, receiver.String()), | ||
| ) |
There was a problem hiding this comment.
do you think it make sense to have variable list of attributes for the same event type?
There was a problem hiding this comment.
why can the receiver be empty? a module has to mint coins somewhere, either to itself or to an address?
There was a problem hiding this comment.
Let's not diverge from the existing event definitions. For minting and sending let's just have two events and not add receiver to the mint event. Better to be consistent with existing usage where possible.
There was a problem hiding this comment.
@fdymylja - receiver is empty to be compatible with the current behavior when minting to a module.
@aaronc - the idea was to not send and have a functionality and event to mint to an account.
Re event: maybe we can add a third argument in both cases? Will it break any client? Do we know how this events are parsed?
There was a problem hiding this comment.
receiver is empty to be compatible with the current behavior when minting to a module.
What does this mean exactly? In what cases is it empty?
the idea was to not send and have a functionality and event to mint to an account.
I don't follow this.
There was a problem hiding this comment.
@robert-zaremba, if the current events are mantained as they are then there's no need.
The current implementation uses the mint event to signal a coinbase
Then the addCoins function creates a CoinReceived event. This combo allows rosetta to be aware of balance changes.
There was a problem hiding this comment.
@fdymylja - my question was rather related to the changes proposed here: keeping same event with one extra argument. I guess, currently Rosetta assumes that all minting is done to a module only, right?
There was a problem hiding this comment.
I guess, currently Rosetta assumes that all minting is done to a module only, right?
Nope, when there's a mint, a coinbase event is produced, which identifies how much the supply was inflated, and who is the minter. This event does not make rosetta add coins to the balance of the minter.
What adds coins to the balance of an address is the event produced by addCoins bank keeper function which is called in the Mint function alongside SendCoins etc...
There was a problem hiding this comment.
I think for consistency we should stick with the current events for now and MintCoinsToAddress would then emit two events - one for minting, one for sending. Does that seem workable @robert-zaremba ?
There was a problem hiding this comment.
OK, so let's do this:
- We don't create a new event nor extend the current one.
- All mint events will not have recipient.
- For Rosetta, to account a new balance, we will use
CoinReceivedEvent-- triggered inaddCoins
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
if we get back to this PR then we should update according to: #10771 |
Description
Closes: #9619
todo:
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
!in the type prefix if API or client breaking change