Per message type fee #376

husio · 2019-03-05T12:21:19Z

Add generic decorator to add optional type-dependent required fees as set in genesis (or updated in bucket)

Please notice that in ac5d8a4 I have introduced changes to the coin package.

- new function `NewCoinp` to create and return a pointer to a coin - `Coin.Add` method allows to add zero value coin without a ticker - update coin tests

Finish the implementation and add tests. resolve #232

alpe · 2019-03-06T12:11:02Z

coin/coin.go

@@ -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


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?

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

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

coin/coin.go

alpe · 2019-03-06T12:22:00Z

coin/coin.go

 	return Coin{
 		Whole:      whole,
 		Fractional: fractional,
 		Ticker:     ticker,
 	}
 }

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


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.

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?

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

alpe · 2019-03-06T12:24:41Z

x/msgfee/codec.proto

+// the message to be processed.
+message MsgFee {
+	string msg_path = 1;
+	coin.Coin fee = 2;


Q: Would it make sense to have the not nil annotation for fee?

pp: fee is a bit overloaded already. This is the product fee to be more precise

Yes, it does make sense to change it to a non-pointer type. 🏃‍♂️

Now that you pointed it out I agree. I think ProductFee is not describing it well. The functionality is to charge for a transaction processing, there is no product 🤔

How about simple Value? MsgFee.Value of type coin.Coin.

I like to think of our features as products especially if they come with a price. I can not think of a better name now. Sorry. If you don't like price then please use fee. It is better than value as it indicates somebody has to pay for this, too. But don't feel blocked by this.

I will stick to the Fee then.
I can be very wrong because I am not a native speaker. price sounds to me like I am paying coins for a product. fee sounds like I am paying coins in addition to the price - an extra amount required as an addition to the transaction.

In the real world when I buy a house the price is 1mln EUR plus extra expences like sales person, writing to the mortgage register, tax. The 1mln is the price and all others are fee. 🤔

x/msgfee/init.go

x/msgfee/model.go

ethanfrey

Great job.
The logic looks solid, and I think it is all correct.
But I added some small code clean-up and a few more test cases to add.

Ping me when done and I can approve it.

ethanfrey · 2019-03-06T13:23:10Z

coin/coin.go

 	return Coin{
 		Whole:      whole,
 		Fractional: fractional,
 		Ticker:     ticker,
 	}
 }

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


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

ethanfrey · 2019-03-06T13:24:59Z

coin/coin.go

@@ -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


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

x/msgfee/decorator.go

ethanfrey · 2019-03-06T13:35:15Z

x/msgfee/init.go

+		MsgPath string    `json:"msg_path"`
+		Fee     coin.Coin `json:"fee"`
+	}
+	var fees []*msgfee


curiosity question:
why do we need []*msgFee rather than []msgFee?

That is a great question!

This is because when iterating over a slice of non-pointers "current" value is always overwritten in each step. This means that if you reference it you will end up with the same value everywhere.

https://play.golang.org/p/diWYP3ElXmF

Nice tip.

It took me a bit to understand where the big came from even after seeing it run.

ethanfrey · 2019-03-06T13:39:04Z

x/msgfee/init_test.go

+
+	db := store.MemStore()
+	var ini Initializer
+	if err := ini.FromGenesis(opts, db); err != nil {


We need to standardize on a test library....

I was using testify/assert, and I think Alex as well.
You use stdlib test, which is quite similar, but I feel requires more typing
GoConvey is quite a break using a different paradigm than table-driven tests.

Note below that you use %s for the error, not %+v, which would show the stacktrace.
And asset.NoError(err) (here require.NoError(err) would automatically give you all this info).

But yes, this has nothing to do with the PR and is a long outstanding issue

There is already an issue that is waiting for a hero to pick up the "fight" #249 ⚔️ 💪

(best to read it while listen to https://www.youtube.com/watch?v=OBwS66EBUcY&t=121)

ethanfrey · 2019-03-06T13:40:47Z

x/msgfee/init_test.go

+	"github.com/iov-one/weave/store"
+)
+
+func TestGenesis(t *testing.T) {


Good happy-path test.
Adding a test with a good error value for an illegal fee would be good... we want to ensure invalid configs die early (on blockchain startup), not on first use. The code does this properly, but I like to see a test... which ensures later changes don't remove the validation accidentally

This is tested already in MsgFee.Validate but ok.

Added in c4ee0bf

ethanfrey · 2019-03-06T13:41:34Z

x/msgfee/decorator_test.go

+	}
+}
+
+var helpers x.TestHelpers


Do you want to extend x.TestHelpers with these functionalities?

I would like to, but preferably after the #372

After #372 is done I am planning to go through the codebase and improve some of the tests.

ethanfrey · 2019-03-06T13:44:25Z

x/msgfee/decorator_test.go

+	"github.com/iov-one/weave/x"
+)
+
+func TestFeeDecorator(t *testing.T) {


Test looks fine, but what about a table-test here to cover a few cases...

not just valid fee present, but also:

no fee present (with and without RequiredFee from the handler)

fee present (with no RequiredFee from handler, with RequiredFee of same token, with RequiredFee of different ticker)

I think that is 5 cases, and you cover 1 here. Some of there are nice edge-cases to ensure working properly (adding with 0 or other currency)

Adressed in a274bf5

I have copied handlerMock because I needed it a274bf5#diff-19ce2e115f45b1e4699811f8c435dbf1R131 I will update this code (remove the copy) once we will complete #372

ethanfrey

Nice improvement. I like.

Just one more test if you can fit it in...
a274bf5#r32617130

ethanfrey · 2019-03-06T15:37:17Z

x/msgfee/decorator_test.go

@@ -29,6 +29,18 @@ func TestFeeDecorator(t *testing.T) {
 			WantCheckFee:   coin.NewCoin(0, 1234, "DOGE"),
 			WantDeliverFee: coin.NewCoin(0, 1234, "DOGE"),
 		},
+		"message fee with addded to existing value with the same currency": {


husio requested a review from ethanfrey March 5, 2019 12:21

wip: #232

43b410e

husio force-pushed the type_dependent_fees_issue_232 branch from a268f01 to 43b410e Compare March 5, 2019 14:50

husio added 2 commits March 6, 2019 11:11

update coin package

ac5d8a4

- new function `NewCoinp` to create and return a pointer to a coin - `Coin.Add` method allows to add zero value coin without a ticker - update coin tests

improve msgfee pakage

c74db3d

Finish the implementation and add tests. resolve #232

husio marked this pull request as ready for review March 6, 2019 10:13

husio requested a review from alpe as a code owner March 6, 2019 10:13

husio removed the request for review from alpe March 6, 2019 10:16

This comment has been minimized.

Sign in to view

alpe reviewed Mar 6, 2019

View reviewed changes

husio changed the title ~~wip: #232~~ Per message type fee Mar 6, 2019

Use coin.Coin as a non nullable attribute.

215557c

ethanfrey reviewed Mar 6, 2019

View reviewed changes

husio added 4 commits March 6, 2019 15:18

pull request review changes

ead18fc

add errors.Wrapf function

28b3cab

extend genesis tests

c4ee0bf

extend decorator tests

a274bf5

ethanfrey approved these changes Mar 6, 2019

View reviewed changes

one moar test

49c792e

ethanfrey reviewed Mar 6, 2019

View reviewed changes

typo

36f162f

husio merged commit c062966 into master Mar 7, 2019

husio deleted the type_dependent_fees_issue_232 branch March 7, 2019 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Per message type fee #376

Per message type fee #376

husio commented Mar 5, 2019 •

edited

Loading

This comment has been minimized.

alpe Mar 6, 2019

husio Mar 6, 2019 •

edited

Loading

ethanfrey Mar 6, 2019

alpe Mar 6, 2019

husio Mar 6, 2019 •

edited

Loading

ethanfrey Mar 6, 2019

alpe Mar 6, 2019

alpe Mar 6, 2019

husio Mar 6, 2019

alpe Mar 6, 2019

husio Mar 7, 2019 •

edited

Loading

ethanfrey left a comment

ethanfrey Mar 6, 2019

ethanfrey Mar 6, 2019

ethanfrey Mar 6, 2019

husio Mar 6, 2019

ethanfrey Mar 6, 2019

ethanfrey Mar 6, 2019

husio Mar 6, 2019 •

edited

Loading

ethanfrey Mar 6, 2019

husio Mar 6, 2019 •

edited

Loading

ethanfrey Mar 6, 2019

husio Mar 6, 2019 •

edited

Loading

ethanfrey Mar 6, 2019

husio Mar 6, 2019 •

edited

Loading

ethanfrey left a comment

ethanfrey Mar 6, 2019

Per message type fee #376

Per message type fee #376

Conversation

husio commented Mar 5, 2019 • edited Loading

This comment has been minimized.

Choose a reason for hiding this comment

husio Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

husio Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

husio Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

husio Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

husio Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

husio Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

husio Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

husio commented Mar 5, 2019 •

edited

Loading

husio Mar 6, 2019 •

edited

Loading

husio Mar 6, 2019 •

edited

Loading

husio Mar 7, 2019 •

edited

Loading

husio Mar 6, 2019 •

edited

Loading

husio Mar 6, 2019 •

edited

Loading

husio Mar 6, 2019 •

edited

Loading

husio Mar 6, 2019 •

edited

Loading