Adding more specialized keepers to bank #831
Conversation
sunnya97
force-pushed
the
sunny/bank-additions
branch
from
c060388 to
52b143d
Compare
Codecov Report
@@ Coverage Diff @@
## develop #831 +/- ##
===========================================
+ Coverage 65.48% 66.79% +1.31%
===========================================
Files 67 67
Lines 3546 3581 +35
===========================================
+ Hits 2322 2392 +70
+ Misses 1034 995 -39
- Partials 190 194 +4 |
sunnya97
requested review from
rigelrozanski and
cwgoes
and removed request for
ebuchman
|
See #802 (comment). |
sunnya97
changed the title
x/bank/keeper_test.go
Outdated
| // wire registration while we're at it ... TODO | ||
| var _ = oldwire.RegisterInterface( | ||
| struct{ sdk.Account }{}, | ||
| oldwire.ConcreteType{&auth.BaseAccount{}, 0x1}, |
There was a problem hiding this comment.
We're on go-amino now :) - let's update these tests to use go-amino
|
|
||
| // ViewKeeper only allows reading of balances | ||
| type ViewKeeper struct { | ||
| am sdk.AccountMapper |
There was a problem hiding this comment.
Do we actually need to create explicit structs for this? Is it not safe for some reason to use just create more limited interfaces and of CoinKeeper and cast a new object to that interface? I'm actually not sure and would like to know
@jaekwon
There was a problem hiding this comment.
More on this I really like @cwgoes design #802 (comment)
There was a problem hiding this comment.
base on 802 conversation ... YES we do want explicit structs - aka this comment is stale
|
Oh yeah also should update the changelog before merge - see you have a checkbox for that |
| func addCoins(ctx sdk.Context, am sdk.AccountMapper, addr sdk.Address, amt sdk.Coins) (sdk.Coins, sdk.Error) { | ||
| oldCoins := getCoins(ctx, am, addr) | ||
| newCoins := oldCoins.Plus(amt) | ||
| if !newCoins.IsNotNegative() { |
There was a problem hiding this comment.
When does this happen - overflow? Should we just panic on overflow? This error would be confusing.
There was a problem hiding this comment.
I was thinking more if someone passes in a negative amount of coins to be added.
There was a problem hiding this comment.
Oh OK, that's fine I guess.
In general, I find representing negative coins a bit confusing (as you never want anyone to end up with negative coins, it's just an intermediate representation, but it uses the same datatype), but maybe that's just me.
There was a problem hiding this comment.
Agreed. Negative coins is weird.
x/bank/keeper.go
Outdated
| } | ||
|
|
||
| // SendCoins moves coins from one account to another | ||
| func (ck CoinKeeper) SendCoins(ctx sdk.Context, fromAddr sdk.Address, toAddr sdk.Address, amt sdk.Coins) sdk.Error { | ||
| _, err := ck.SubtractCoins(ctx, fromAddr, amt) | ||
| func sendCoins(ctx sdk.Context, am sdk.AccountMapper, fromAddr sdk.Address, toAddr sdk.Address, amt sdk.Coins) sdk.Error { |
There was a problem hiding this comment.
Can we be explicit (in the comment) that if an error is returned, the caller must revert state changes (otherwise coins could be subtracted but not added).
There was a problem hiding this comment.
Or at least the CacheWrap should be thrown out. That should be the general behavior upon error.
There was a problem hiding this comment.
I thought this was going to be handled by BaseApp or something?
There was a problem hiding this comment.
It is - just make it clear in the comment so if someone writes a different baseapp they don't accidentally destroy coins
x/bank/keeper.go
Outdated
| @@ -73,20 +69,119 @@ func (ck CoinKeeper) SendCoins(ctx sdk.Context, fromAddr sdk.Address, toAddr sdk | |||
| } | |||
|
|
|||
| // InputOutputCoins handles a list of inputs and outputs | |||
| func (ck CoinKeeper) InputOutputCoins(ctx sdk.Context, inputs []Input, outputs []Output) sdk.Error { | |||
| func inputOutputCoins(ctx sdk.Context, am sdk.AccountMapper, inputs []Input, outputs []Output) sdk.Error { | |||
There was a problem hiding this comment.
Ditto on above - if an error is returned, the caller must revert state changes.
|
|
||
| } | ||
|
|
||
| func TestViewKeeper(t *testing.T) { |
There was a problem hiding this comment.
Add tests to make sure ViewKeeper can't be cast to SendKeeper.
There was a problem hiding this comment.
They are currently both structs, so can't cast them to each other already. Won't compile.
| // CoinKeeper manages transfers between accounts | ||
| type CoinKeeper struct { | ||
| am sdk.AccountMapper | ||
| func getCoins(ctx sdk.Context, am sdk.AccountMapper, addr sdk.Address) sdk.Coins { |
There was a problem hiding this comment.
This extra method indirection is not useful, I think we should just define these as CoinKeeper methods.
There was a problem hiding this comment.
But then how would I call it from the ViewKeeper for example?
There was a problem hiding this comment.
I could embed a CoinKeeper in ViewKeeper, but I would prefer to not have to create a new CoinKeeper just to create a ViewKeeper.
There was a problem hiding this comment.
Can you embed the ViewKeeper in the CoinKeeper though? The creation can happen in NewCoinKeeper, which is already an exported function.
There was a problem hiding this comment.
I don't like embedding structs because if you have "diamond inheritance" it cries unambiguous selector. https://play.golang.org/p/-t7jTejvaTg
I think the unexported functions in the module which each keeper can call is cleanest and most versatile without ever having to think about inheritance issues.
cwgoes
force-pushed
the
sunny/bank-additions
branch
from
500f71b to
d2013ec
Compare
Closes #802
Also added test coverage to bank keepers.