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.

Sign up for GitHub

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

Per message type fee #376

Merged
merged 10 commits into from
Mar 7, 2019
Merged

Per message type fee #376

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
43b410e
wip: #232
husio Mar 5, 2019
ac5d8a4
update coin package
husio Mar 6, 2019
c74db3d
improve msgfee pakage
husio Mar 6, 2019
215557c
Use coin.Coin as a non nullable attribute.
husio Mar 6, 2019
ead18fc
pull request review changes
husio Mar 6, 2019
28b3cab
add errors.Wrapf function
husio Mar 6, 2019
c4ee0bf
extend genesis tests
husio Mar 6, 2019
a274bf5
extend decorator tests
husio Mar 6, 2019
49c792e
one moar test
husio Mar 6, 2019
36f162f
typo
husio Mar 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ protoc:
protoc --gogofaster_out=. -I=. -I=./vendor -I=$(GOPATH)/src cmd/bnsd/x/nft/username/*.proto
protoc --gogofaster_out=. -I=. -I=$(GOPATH)/src x/cash/*.proto
protoc --gogofaster_out=. -I=. -I=$(GOPATH)/src x/sigs/*.proto
protoc --gogofaster_out=. -I=. -I=$(GOPATH)/src x/msgfee/*.proto
protoc --gogofaster_out=. -I=. -I=./vendor -I=$(GOPATH)/src x/multisig/*.proto
protoc --gogofaster_out=. -I=. -I=./vendor -I=$(GOPATH)/src x/validators/*.proto
protoc --gogofaster_out=. -I=. -I=$(GOPATH)/src x/batch/*.proto
Expand Down
23 changes: 20 additions & 3 deletions coin/coin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,20 @@ const (
)

// NewCoin creates a new coin object
func NewCoin(whole int64, fractional int64,
ticker string) Coin {

func NewCoin(whole int64, fractional int64, ticker string) Coin {
return Coin{
Whole: whole,
Fractional: fractional,
Ticker: ticker,
}
}

// NewCoinp returns a pointer to a new coin.
func NewCoinp(whole, fractional int64, ticker string) *Coin {
Copy link
Contributor

Choose a reason for hiding this comment

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

pp: I don't see big value in adding another constructor with pointer result for the tests. I would suggest you make coin a non null type in the protobuf via annotations so that you don't have to deal with this case.

Copy link
Contributor Author

@husio husio Mar 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

There are multiple places where coinp is declared. This is the fourth time I was declaring it so I thought it deserves its own function.

Using non null coin reference is problematic because I want to compare with a coin pointer. This is the case in many places where a result is a coin pointer and that is what you want to check becuase a result can be both a zero value coin instance and nil.

Does this make sense?

I have pushed a change where I replace pointer to a coin with embedded value 215557c

I still think that coin.NewCoinp is useful in other cases and could be used instead of ad-hoc creation of coinp function. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have seen (and used) this work-around in lots of test code. Fair enough to bring it here as a helper.

c := NewCoin(whole, fractional, ticker)
return &c
}

// ID returns a coin ticker name.
func (c Coin) ID() string {
return c.Ticker
Expand Down Expand Up @@ -128,10 +132,20 @@ func mul64(a, b int64) (int64, error) {
// currencies, or if the combination would cause
// an overflow
func (c Coin) Add(o Coin) (Coin, error) {
// If any of the coins represents no value and does not have a ticker
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: a coin without a ticker is an invalid coin. even if it is zero, IMHO. Is this code due to #369 not done yet?

Copy link
Contributor Author

@husio husio Mar 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

That is a good point.

This code is because of https://github.com/iov-one/weave/pull/376/files#diff-e1c2020236c87568b4fce1d6a33e620cR39

It is often in weave codebase that instead of a pointer a zero value is returned and later used, for example weave.CheckResult or mentioned coin.Coin. If they are returned together with an error value, they are not a pointer but an empty structure.
The above cause sometimes weird situations where a value is zero (not nil). Because it is not nil, all methods should apply (it is a value right?). And then you are getting into a complicated question of "does this make sense?" 😁

In this case I think it does make sense. My logic was

0     + 1 BTC = 1 BTC  // zero value
0 BTC + 1 BTC = 1 BTC
0 BTC + 0 ETH = error
1 BTC + 0 ETH = error
0 BTC + 1 ETH = error

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the zero value, uninitialized struct.
Since we don't use pointers everywhere, and we mean 'noop' / 'do nothing' when returning coin.Coin{}, we can make the functions treat it special

// set then it has no influence on the addition result.
if c.Ticker == "" && c.IsZero() {
return o, nil
}
if o.Ticker == "" && o.IsZero() {
return c, nil
}

if !c.SameType(o) {
err := ErrInvalidCurrency.Newf("adding %s to %s", c.Ticker, o.Ticker)
return Coin{}, err
}

c.Whole += o.Whole
c.Fractional += o.Fractional
return c.normalize()
Expand Down Expand Up @@ -225,6 +239,9 @@ func (c Coin) SameType(o Coin) bool {

// Clone provides an independent copy of a coin pointer
func (c *Coin) Clone() *Coin {
if c == nil {
husio marked this conversation as resolved.
Show resolved Hide resolved
return nil
}
return &Coin{
Ticker: c.Ticker,
Whole: c.Whole,
Expand Down
87 changes: 53 additions & 34 deletions coin/coin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,44 +155,63 @@ func TestValidCoin(t *testing.T) {

func TestAddCoin(t *testing.T) {
base := NewCoin(17, 2345566, "DEF")
cases := []struct {
a, b Coin
res Coin
bad bool
cases := map[string]struct {
a, b Coin
wantRes Coin
wantErr error
}{
// plus and minus equals 0
{base, base.Negative(), NewCoin(0, 0, "DEF"), false},
// wrong types
{
NewCoin(1, 2, "FOO"),
NewCoin(2, 3, "BAR"),
Coin{},
true,
},
// normal math
{
NewCoin(7, 5000, "ABC"),
NewCoin(-4, -12000, "ABC"),
NewCoin(2, 999993000, "ABC"),
false,
},
// overflow
{
NewCoin(500500500123456, 0, "SEE"),
NewCoin(500500500123456, 0, "SEE"),
Coin{},
true,
"plus and minus equals 0": {
a: base,
b: base.Negative(),
wantRes: NewCoin(0, 0, "DEF"),
},
"wrong types": {
a: NewCoin(1, 2, "FOO"),
b: NewCoin(2, 3, "BAR"),
wantRes: Coin{},
wantErr: ErrInvalidCurrency,
},
"normal math": {
a: NewCoin(7, 5000, "ABC"),
b: NewCoin(-4, -12000, "ABC"),
wantRes: NewCoin(2, 999993000, "ABC"),
},
"overflow": {
a: NewCoin(500500500123456, 0, "SEE"),
b: NewCoin(500500500123456, 0, "SEE"),
wantRes: NewCoin(0, 0, ""),
wantErr: ErrInvalidCoin,
},
"adding to zero coin": {
a: NewCoin(0, 0, ""),
b: NewCoin(1, 0, "DOGE"),
wantRes: NewCoin(1, 0, "DOGE"),
},
"adding a zero coin": {
a: NewCoin(1, 0, "DOGE"),
b: NewCoin(0, 0, ""),
wantRes: NewCoin(1, 0, "DOGE"),
},
"adding a non zero coin without a ticker": {
a: NewCoin(1, 0, "DOGE"),
b: NewCoin(1, 0, ""),
wantErr: ErrInvalidCurrency,
},
"adding to non zero coin without a ticker": {
a: NewCoin(1, 0, ""),
b: NewCoin(1, 0, "DOGE"),
wantErr: ErrInvalidCurrency,
},
}

for idx, tc := range cases {
t.Run(fmt.Sprintf("case-%d", idx), func(t *testing.T) {
c, err := tc.a.Add(tc.b)
if tc.bad {
assert.Error(t, err)
} else {
assert.NoError(t, err)
assert.Equal(t, tc.res, c)
for testName, tc := range cases {
t.Run(testName, func(t *testing.T) {
res, err := tc.a.Add(tc.b)
if !errors.Is(tc.wantErr, err) {
t.Fatalf("got error: %v", err)
}
if tc.wantErr == nil {
assert.Equal(t, tc.wantRes, res)
}
})
}
Expand Down
Loading