Implement fee handler function in AnteHandler, add default BurnFeeHandler #852

cwgoes · 2018-04-12T11:04:41Z

Ref #583 - this just implements the AccountMapper part of that issue.

codecov · 2018-04-12T11:07:46Z

Codecov Report

Merging #852 into develop will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop     #852      +/-   ##
===========================================
+ Coverage    65.45%   65.48%   +0.02%     
===========================================
  Files           67       67              
  Lines         3543     3546       +3     
===========================================
+ Hits          2319     2322       +3     
  Misses        1034     1034              
  Partials       190      190

rigelrozanski · 2018-04-12T19:48:16Z

x/auth/ante.go

@@ -74,6 +74,9 @@ func NewAnteHandler(accountMapper sdk.AccountMapper) sdk.AnteHandler {
 				// TODO: min fee
 				if !fee.Amount.IsZero() {
 					signerAcc, res = deductFees(signerAcc, fee)
+					pool := accountMapper.GetFeePool(ctx)


I'm not a fan of extending accountMapper in this way - I reckon we should keep accountMapper as is and have AnteHandler take a new type of keeper (which can be defined in /types) which the stake module will define. The AnteHandler can have an additional function input say "FeeHandler" has a method HandleFees(ctx, fee coin.Coins) which actually performs the movement of the fees. HandleFees could be called right here

But yeah then the reserve tax can be implemented as a part of HandleFees too

The nice part about this implementation is that it's cheap (one read, one write) and complex fee redistribution can be performed later in a batch at the end of the block - but I'll implement that option in another PR, then we can see which we like best.

yeah I mean it's actually not cheap because now we need to handle the fees twice, first to write to this pool then a second time to digest. I'd like us to tackle both simultaneously - it's important to optimize here as it's every block that these operations are called

Hmm, according to https://cosmos.network/whitepaper fees are redistributed every ValidatorPayoutPeriod (default 1 hour), so it seems like we don't need to do that every block?

Even if we did redistribute every block instead, any implementation not writing to intermediary state (which could be in the AccountMapper, or not) would have to redistribute every transaction, which seems like it would be very inefficient (read/write all the bonded stakeholder bond values and accounts)?

@rigelrozanski what do you mean by "digest"?

@sunnya97 apologies for the vague language - yeah digest = process the fee pool for the the proposer as outlined in the spec - https://github.com/cosmos/cosmos-sdk/blob/master/docs/spec/staking/old/spec2.md#fee-calculations

@cwgoes Also, due to technical reasons we must vear from the whitepaper proposed hourly distribution. Talked with Jae about this before - is OK. But yeah digest doesn't mean we're "distributing" to the validators we're calculating how the "feepool shares" ought to be modified for the proposer validator - this calculation is dependant on the fees collected in each block/digest. So yeah, the staking fee calculation actually needs access to what each incoming fee is - if it only has access to the pool in this current implementation, it would actually need to keep a second copy of the pool to calculate the difference, which is totally nuts when we could just do it all at once

Refactored to feeHandler

jaekwon · 2018-04-16T12:32:27Z

examples/basecoin/app/app.go

@@ -60,6 +63,9 @@ func NewBasecoinApp(logger log.Logger, db dbm.DB) *BasecoinApp {
 		&types.AppAccount{}, // prototype
 	).Seal()

+	// Define the feeHandler.
+	app.feeHandler = func(ctx sdk.Context, fees sdk.Coins) {}


Should be defined below coinKeeper? Would be nice to have the Tx too.

Then the function body would reference coinKeeper as a closure. Same with democoin.

cwgoes · 2018-04-16T12:59:06Z

Updated per comments

jaekwon · 2018-04-16T13:06:35Z

examples/basecoin/app/app.go

@@ -75,6 +72,9 @@ func NewBasecoinApp(logger log.Logger, db dbm.DB) *BasecoinApp {
 		AddRoute("ibc", ibc.NewHandler(ibcMapper, coinKeeper)).
 		AddRoute("simplestake", simplestake.NewHandler(stakeKeeper))

+	// Define the feeHandler.
+	app.feeHandler = func(ctx sdk.Context, tx sdk.Tx, fees sdk.Coins) {}


needs implementation or a new Issue, same with democoin.

Yes, will be implemented by the stake module, I guess this PR can just wait for that, no reason to merge beforehand

We shouldn't wait to merge this for staking implementation - let's add a basic "burn the fees" implementation of feeHandler as a part of this PR and merge

That's what this does at the moment (the fees are burned and the total supply is decreased).

Did you want the handler to do something else (log the burn somehow)?

nah I think that's fine - will review one last time and then merge soon unless something comes up

rigelrozanski · 2018-04-16T21:54:52Z

x/auth/ante_test.go

@@ -12,6 +12,9 @@ import (
 	wire "github.com/cosmos/cosmos-sdk/wire"
 )

+func nopFeeHandler(ctx sdk.Context, tx sdk.Tx, fee sdk.Coins) {


what does nop stand for? Why not call it BurnFeeHandler and have the apps and other examples import it?

Good idea, renamed BurnFeeHandler, basecoin & democoin now import

cwgoes requested a review from ebuchman as a code owner April 12, 2018 11:04

cwgoes requested review from sunnya97 and rigelrozanski and removed request for ebuchman April 12, 2018 11:15

rigelrozanski previously requested changes Apr 12, 2018

View reviewed changes

cwgoes added 2 commits April 16, 2018 13:59

Implement fee pool pseudo-account in sdk.AccountMapper

e870163

Add testcases

7fafa9e

cwgoes force-pushed the cwgoes/fee-account branch from 85c3a0b to 7fafa9e Compare April 16, 2018 12:01

cwgoes changed the title ~~Implement fee pool pseudo-account in sdk.AccountMapper~~ Add feeHandler function to AnteHandler Apr 16, 2018

Refactor to use FeeHandler function

39ee95a

jaekwon reviewed Apr 16, 2018

View reviewed changes

Add tx to feeHandler, move around in example apps

3bbb15f

jaekwon reviewed Apr 16, 2018

View reviewed changes

cwgoes changed the title ~~Add feeHandler function to AnteHandler~~ WIP: Staking fee implementation Apr 16, 2018

rigelrozanski reviewed Apr 16, 2018

View reviewed changes

Switch to BurnFeeHandler

474fa1d

cwgoes changed the title ~~WIP: Staking fee implementation~~ Implement fee handler function in AnteHandler, add default BurnFeeHandler Apr 16, 2018

Update changelog

e3045fd

rigelrozanski approved these changes Apr 16, 2018

View reviewed changes

cwgoes merged commit 0f2aa6b into develop Apr 16, 2018

cwgoes deleted the cwgoes/fee-account branch April 16, 2018 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement fee handler function in AnteHandler, add default BurnFeeHandler #852

Implement fee handler function in AnteHandler, add default BurnFeeHandler #852

cwgoes commented Apr 12, 2018

codecov bot commented Apr 12, 2018 •

edited

Loading

rigelrozanski Apr 12, 2018

rigelrozanski Apr 12, 2018

cwgoes Apr 12, 2018 •

edited

Loading

rigelrozanski Apr 13, 2018 •

edited

Loading

cwgoes Apr 13, 2018 •

edited

Loading

sunnya97 Apr 13, 2018

rigelrozanski Apr 13, 2018 •

edited

Loading

jaekwon Apr 16, 2018

jaekwon Apr 16, 2018

cwgoes commented Apr 16, 2018

jaekwon Apr 16, 2018

cwgoes Apr 16, 2018

rigelrozanski Apr 16, 2018

cwgoes Apr 16, 2018

cwgoes Apr 16, 2018

rigelrozanski Apr 16, 2018

rigelrozanski Apr 16, 2018

cwgoes Apr 16, 2018

Implement fee handler function in AnteHandler, add default BurnFeeHandler #852

Implement fee handler function in AnteHandler, add default BurnFeeHandler #852

Conversation

cwgoes commented Apr 12, 2018

codecov bot commented Apr 12, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

rigelrozanski Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

cwgoes Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rigelrozanski Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Apr 16, 2018

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

Choose a reason for hiding this comment

codecov bot commented Apr 12, 2018 •

edited

Loading

cwgoes Apr 12, 2018 •

edited

Loading

rigelrozanski Apr 13, 2018 •

edited

Loading

cwgoes Apr 13, 2018 •

edited

Loading

rigelrozanski Apr 13, 2018 •

edited

Loading