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

fees #615

Merged
merged 10 commits into from
Mar 17, 2018
Merged

fees #615

merged 10 commits into from
Mar 17, 2018

Conversation

ebuchman
Copy link
Member

@ebuchman ebuchman commented Mar 13, 2018

#613

Also elements from #483 / #562

Currently it's just the data structures. Need to actually implement the logic and write tests still.

@sunnya97 @jaekwon any feedback ?

@ebuchman ebuchman changed the title Bucky/fees WIP: fees Mar 13, 2018
@codecov
Copy link

codecov bot commented Mar 13, 2018

Codecov Report

Merging #615 into develop will increase coverage by 0.72%.
The diff coverage is 58.33%.

@@             Coverage Diff             @@
##           develop     #615      +/-   ##
===========================================
+ Coverage    62.42%   63.14%   +0.72%     
===========================================
  Files           34       34              
  Lines         1948     1997      +49     
===========================================
+ Hits          1216     1261      +45     
- Misses         629      634       +5     
+ Partials       103      102       -1

@adrianbrink
Copy link
Contributor

I like this. Only nitpick is that we should pass in the Fees in the constructor NewStdTx).

@ebuchman
Copy link
Member Author

ebuchman commented Mar 14, 2018

I like this. Only nitpick is that we should pass in the Fees in the constructor NewStdTx).

Yeh, I just didn't want to break the API for now.

@mappum want to take over ? (small contained issue #622)

@ebuchman
Copy link
Member Author

Rebased on #634

Just need to finish up some tests !

if !res.IsOK() {
return ctx, res, true
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

get rid?

x/auth/ante.go Outdated
return
}

// deduct the fee from the account
func deductFees(acc sdk.Account, fee sdk.StdFee) (sdk.Account, sdk.Result) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add note as to why not using coin keeper

@ebuchman ebuchman changed the title WIP: fees fees Mar 17, 2018
return sdk.Coins{
{"atom", 10000000},
}
}
Copy link
Contributor

@rigelrozanski rigelrozanski Mar 17, 2018

Choose a reason for hiding this comment

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

I'd consider getting rid of this function and just declaring coins each time, annoying to have to look it up doesn't even really save space because can define in one line ... newCoins() isn't that much smaller than sdk.Coins{{atom, 1000}} just more obscure

}{
{"", []int64{1}, fee, msg, codeUnauth}, // test invalid chain_id
{chainID, []int64{2}, fee, msg, codeUnauth}, // test wrong seqs
{chainID, []int64{1}, fee, newTestMsg(addr2), codeUnauth}, // test wrong msg
Copy link
Contributor

Choose a reason for hiding this comment

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

test bad fee


assert.EqualValues(t, addr, acc.GetAddress())
// cant override pubkey
Copy link
Contributor

Choose a reason for hiding this comment

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

can't


err := acc.SetPubKey(pub)
// cant override address
Copy link
Contributor

Choose a reason for hiding this comment

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

can't

acc := NewBaseAccountWithAddress(addr)

assert.Panics(t, func() { acc.Get("key") })
assert.Panics(t, func() { acc.Set("key", "value") })
Copy link
Contributor

Choose a reason for hiding this comment

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

could use more comments here - not totally obvious why this should panic... is there a case where it doesn't panic and can we include that?

Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

tests pass

@ebuchman ebuchman merged commit 849c12c into develop Mar 17, 2018
@ebuchman ebuchman deleted the bucky/fees branch March 17, 2018 21:51
This was referenced Mar 17, 2018
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.

4 participants