WIP: Feature/transaction fees

[ruseinov]

Fixes #232

I have reiterated on this a couple of times and it seems like the best solution is to init from genesis and not gconf, as we need to support lookups.

Also, the current idea for dynamic fees is as follows:
for static fees we just use nft/approval/grant
for dynamic fees we go nft/approval/grant:unique_name

There is no reason to complicate this further as this logic could be easily defined in our controllers and that well save us the space and the indices, allow for easy fallbacks and is pretty flexible.

I'd like to hear some thoughts about that though before continuing.

[ruseinov]

The idea for dynamic fees is we define a bunch of business logic that will be executed in the fee decorator, that will match messages by their type and use whatever unique properties (or not unique) to construct an id out of Path() + this info for lookups, with fallback to the original Path().

I think this will be a sort of a callback passed directly from the implementing app to avoid dependencies on other packages in fee package.

[codecov-io]

Codecov Report

Merging #323 into master will increase coverage by <.1%.
The diff coverage is 77.7%.

@@           Coverage Diff            @@
##           master    #323     +/-   ##
========================================
+ Coverage    69.5%   69.6%   +<.1%     
========================================
  Files         134     134             
  Lines        6183    6219     +36     
========================================
+ Hits         4302    4330     +28     
- Misses       1468    1472      +4     
- Partials      413     417      +4

Impacted Files	Coverage Δ
x/cash/init.go	67.7% <73.3%> (+5.2%)	⬆️
x/cash/model.go	84.6% <80.9%> (-1.4%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 483fb2b...6a97e34. Read the comment docs.

[ruseinov]

@ethanfrey @alpe After taking another look at this I've moved my initial fee logic to cash, cause it seems like we'll need the FeeTx and the logic to figure out who pays to whom.

This raises a couple of concerns, one of which being the following: we want this to support batch and be downstream from it, but we have cash.FeeDectorator upstream from hashlock, which is a conflict then.

Looking into it further, but would appreciate some input on the approach.

[husio]

@ruseinov can you explain a bit more how your implementation will work? I can see only state management code and no actual fee collection.

My understanding of this functionality is that we have the possibility to declare fees for each transaction type. Somewhere we keep a mapping of message type -> coins. For example processing paychan.CreatePaymentChannelMsg costs 7 IOV.
An assumption that I am making is that transaction fees extension is self contained and does not require any external integration. This means that no other extension must change in order for the fees to be collected. If tomorrow an extension foo is created, I can already today register foo.BarMsg costs 42 IOV and this fee will be collected as soon as the message is used.

Does it make sense to keep this functionality as a separate extension and not part of the x/cash?

[ethanfrey]

I have reiterated on this a couple of times and it seems like the best solution is to init from genesis and not gconf, as we need to support lookups.

After reflection I agree. This is not a singleton config, but a bucket with a different entry for each tx type with fees registered

[ethanfrey]

Also, the current idea for dynamic fees is as follows:
for static fees we just use nft/approval/grant
for dynamic fees we go nft/approval/grant:unique_name

I would ignore all thinking for what you call "dynamic fees" or fees that depend on the content, not just the type of the message. For static fees, I agree with this.

[ethanfrey]

After taking another look at this I've moved my initial fee logic to cash, cause it seems like we'll need the FeeTx and the logic to figure out who pays to whom.

Note from the issue:

• All product fees go to same location as the anti-spam fees (maybe unify these in one module for easier config)

All fees go to the same location. This basically sets a higher filter for transactions that contain messages with product fees

[ethanfrey]

This raises a couple of concerns, one of which being the following: we want this to support batch and be downstream from it, but we have cash.FeeDectorator upstream from hashlock, which is a conflict then.

Huh?

This only is a problem as you want to re-use the cash.FeeDecorator (the anti-spam measure) for a product fee filter.... why not another decorator.

Since you hit some design limits, I would appreciate you documenting the current plan better before proceeding with the code, so I can give a higher level feedback. I can give good pointer then, we talking about where is calculated and where does it flow...

[ethanfrey]

btw, good that you started this to give time for comments on the design, but please finish up the errors refactor before investing much more time coding this, and giving a bit of time for design feedback

[ruseinov]

@ethanfrey I started this exactly because it is never 100% clear on how to do anything before you approach it + I was stuck on my first error pr review.

So this is now a good placeholder for discussions, while I can focus on errors, when I'm blocked - I circle back here.

[ruseinov]

@husio there is no code for transactions yet, because we needed to have a discussion first, hence this PR with a bunch of questions and the only implementation detail I know for sure won't change.

[ruseinov]

@ethanfrey So in my understanding the hashlock is going to be a problem for any kind of fees, not just anti-spam.
I don't see a good reason to introduce another decorator as we need to duplicate the code to figure out who pays to whom, so I think the whole fee deduction logic actually belongs in on decorator.

The idea is:

1. We specify product fees via genesis and load into bucket
2. 1 allows us to query this from the client after registering the bucket
3. We introduce fee transfer next to anti-spam fee logic with payee and target account already defined
4. voila!

[ethanfrey]

hashlock is going to be a problem for any kind of fees, not just anti-spam.

When using a hash lock, the transaction still has a signature (that pays the fees) and a hash preimage (to authorize the release). The hash lock never pays fees. In fact it can go anywhere above batch

Or even after batch, just duplicate work

[ruseinov]

@ethanfrey right, I thought we could move it too. in this case I would argue that having a combined decorator makes more sense, as the base code is mostly the same, or at least I expect it to be.

[ethanfrey]

I mostly agree with the above comment, but have some additions to keep them separate, cuz really, most chains have no product fee, while almost all have anti spam

[ruseinov]

Well, that’s easily pluggable, we can simply turn off that feature if it’s not configured in genesis.

If that does not make sense - I can try a base handler approach to share some of this code or similar.

[ethanfrey]

In short, add a field to CheckResult and Deliver Result (they are weave internal) for minimum allowed fee.

On the way in the fee filter checks against anti spam fee. On the way out it checks against return value (if the handlers set this)

Each handler can add the antispam fee. Or we can make a simple decorator beyond batch that does the lookup and adds to the result, and then add it in batch just like gas price.

Somehow I prefer to keep the required fee logic generic in the fee Middleware and focus on the transfer and enforcing. And then calculate needed fees elsewhere in an app dependent manner, where it is easy to extend for dynamic fees

[ruseinov]

I’m not sure I got it right. So do you want to keep anti-spam as is and deal with product fee elsewhere?

I think the approach with putting this beyond batch makes more sense then, as it’s easier to understand and implement.

Let’s do this.

[ethanfrey]

I’m not sure I got it right. So do you want to keep anti-spam as is and deal with product fee elsewhere?

Exactly.

[ethanfrey]

I was trying to work out the above comment a bit more (wrote on the phone while waiting for my son). I still agree with most of it, but could flesh it out.

1. Fee wrapper deducts any sent fees and sends them to the fee collector. (as now)
2. The fee wrapper can also enforce the minimum "fixed anti-spam" fee
3. We add a field to the result for "required fee".
4. We let each Handler/Middleware optionally return a value in this field (and batch can sum them)
5. We can provide an extension "product fee" with bucket and initializer that stores some info.
6. We can either:
   a) Allow each handler to call into this "product fee" extension to calculate the fee for a message, eg. res.RequiredFee += productfee.LookupFee(db, msg.Path()) . This is a bit more code, but allows us to do "dynamic fees" with deep introspection easily.
   b) Add a middleware towards the bottom of the stack that does this lookup for every message
   c) Some combination of the above... eg (b) for constant fees (a) when we need to define it per-message not per-type
7. We add some Middleware to enforce this RequiredFee by checking it against the Tx, which is available everywhere, and return an error if insufficent (this would be after processing all tx, but usually this would fail in CheckTx where we only do light-weight processing).

I think this leaves a clean separation of concerns, and if someone wants to add new fee requirements, this would be orthogonal. Such as a module that adds a subjective CheckTx requirement that can be adjusted by each validator (this is what they did in Game of Stakes). It would just set this RequiredFee field.

Sounds good?

I see just one problem here, which is that the FeeMiddleware is outside the Checkpoint. Which means if a tx gets in the block with a fee that is bigger than the anti-spam fee, but not as big as the product fee, it will fail, but still lose the fee.

Actually, this is a problem in any case, we deduct the entire fee in any case. Maybe we don't have to worry about this. Maybe there is a clever idea to fix it?

Anyway, this is more or less my design. Happy for improvements.

[ruseinov]

The approach sounds good, I need to think a bit more about it to see what option I'd prefer, I also think this will require some light prototyping to see what looks most eloquent.

Let me think about the checkpoint issue a bit more too.

[ethanfrey]

Let me think about the checkpoint issue a bit more too.

I mean, it is also good to define expectations here...

With the current setup (anti-spam), insufficient fees cause rejection in CheckTx and never make it to a block. If it does make it into a block, all fees are deducted (assuming the fee account owner has signed) before executing any transactions, and are not rolled back on failure. The entire amount of fees are deducted in any case.

Before working out product fee behavior, maybe we should consider if the above is optimal.

Another approach:

• On error, we only ever deduct the minimum anti-spam fee (as we rollback all work)
• On success, we deduct the entire fee amount sent with the transaction
  Not sure if it is better. It is "nicer" to people who made mistakes, but maybe more open to abuse. It is also more compatible with what we would desire from product fee behavior, I believe.

Maybe there is another approach (not technical, but business logic-level) of how we deduct fees. @alpe maybe you have some thoughts here about fee deduction

[alpe]

I like the idea of adding a RequiredFee field to the result very much. This would give us a lot of flexibility on how fees are calculated and separates the calculation(s) from the deduction logic. Very good design!

In normal business I would say let's take all the fees on error as well, prepare a budget and let customer support handle the edge cases until some new learnings can be applied to the code.
Here I would say, let's do the opposite. It would be hard for us to get in touch with the users to handle their cases and complains. If we want to provide a good experience we need to be more generous and only take the anti-spam fee for example. We could build reports based on the blockchain data and and compensate the validators if this is ever a case. Also feedback from the validators should be easier to get.
A special case would be batch msg if the anti-spam fee does not take the payload into account. But batch is not enabled with BNS.

[ruseinov]

Yeah, I like the idea of deducting just the anti-spam fee too I guess

[ethanfrey]

Simple adjustment to the fee handler:

On the way in:

1. ensure fee is greater than anti-spam fee
2. ensure there is money to cover the entire fee
3. remove anti-spam fee from account (this will be committed along with nonce even in rollback)

On the way out:

1. If not an error, remove the rest of the fee

However, if my transaction empties my account and I cannot cover my fee at the end, the transaction logic was still committed and not rolled back.

---

Maybe we can adjust this...

More complex adjustment to the fee handler:

On the way in:

1. ensure fee is greater than anti-spam fee
2. remove anti-spam fee from account to fee collector
3. move rest of the fee to some temporary address (can be eg cash/fee/1 condition)

On the way out:

1. If not an error, return the money from temporary account to the tx sender
2. If error, send the money from temporary account to the fee collector

A bit trickier, but more correct I think

[ethanfrey]

Good discussion, after this we agreed on a new approach, documented in #232 under "update".

Closing this one in favor of other PRs that take a different approach with fees