refactor!: manual payout only #68
Conversation
Reecepbcups
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #68 +/- ##
==========================================
+ Coverage 72.31% 72.94% +0.63%
==========================================
Files 33 31 -2
Lines 1795 1678 -117
==========================================
- Hits 1298 1224 -74
+ Misses 432 403 -29
+ Partials 65 51 -14 ☔ View full report in Codecov by Sentry. |
Reecepbcups
changed the title
There was a problem hiding this comment.
Thanks, @Reecepbcups, this is awesome!
Some small comments and nit, nothing major.
| return fmt.Errorf("invalid payout: %v for address: %s", p, addr) | ||
| } | ||
|
|
||
| if err := k.mintCoinsToAccount(ctx, sdkAddr, coin); err != nil { |
There was a problem hiding this comment.
Am I correct to believe this function still allows partial minting if an error occurs in a later iteration?
Can we mitigate this by performing all the address and coin checks first?
The operation can still fail for insert reason in the SDK. Is there a way to avoid partial minting at all?
There was a problem hiding this comment.
Partial minting is not possible in the SDK. Either everything in the Tx is successful for none of it is. Atomnic Txs are not yet in the SDK. maybe SDK v0.52
| }, | ||
| { | ||
| desc: "duplicate address", |
There was a problem hiding this comment.
We should keep this check and test. Duplicate addresses should be avoided.
There was a problem hiding this comment.
Added duplicate test to validate (CLI) and in the MsgServer. Also added to e2e to validate it's deterministic (i.e. reason I did not use a map given the last issue we had, I still need to look more into)
| if c.shouldFail { | ||
| require.Error(t, err) | ||
| return |
There was a problem hiding this comment.
We should check the balances on failure to see if we didn't perform partial minting.
There was a problem hiding this comment.
ref: partial minting is not possible in the SDK - #68 (comment)
closes #56
closes #27
Summary
TODO
make local-imageandmake local-image-covis confusing. We should just use -cov only and have it namedlocal-image