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

R4R: Cap(clip) reward to remaining coins in AllocateTokens #3726

Merged
merged 1 commit into from
Feb 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 PENDING.md
Original file line number Diff line number Diff line change
Expand Up @@ -96,5 +96,6 @@ CLI flag.
where validator is unexpectedly slashed throwing off test calculations
* [\#3411] Include the `RequestInitChain.Time` in the block header init during
`InitChain`.
* [\#3726] Cap(clip) reward to remaining coins in AllocateTokens.

### Tendermint
14 changes: 14 additions & 0 deletions types/dec_coin.go
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,20 @@ func (coins DecCoins) SafeSub(coinsB DecCoins) (DecCoins, bool) {
return diff, diff.IsAnyNegative()
}

// Trims any denom amount from coin which exceeds that of coinB,
// such that (coin.Cap(coinB)).IsLTE(coinB).
func (coins DecCoins) Cap(coinsB DecCoins) DecCoins {
res := make([]DecCoin, len(coins))
for i, coin := range coins {
minCoin := DecCoin{
Denom: coin.Denom,
Amount: MinDec(coin.Amount, coinsB.AmountOf(coin.Denom)),
}
res[i] = minCoin
}
return removeZeroDecCoins(res)
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
}

// IsAnyNegative returns true if there is at least one coin whose amount
// is negative; returns false otherwise. It returns false if the DecCoins set
// is empty too.
Expand Down
32 changes: 32 additions & 0 deletions types/dec_coin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,3 +224,35 @@ func TestDecCoinsString(t *testing.T) {
require.Equal(t, tc.expected, out, "unexpected result for test case #%d, input: %v", i, tc.input)
}
}

func TestDecCoinsCap(t *testing.T) {
testCases := []struct {
input1 string
input2 string
expectedResult string
}{
{"", "", ""},
{"1.0stake", "", ""},
{"1.0stake", "1.0stake", "1.0stake"},
{"", "1.0stake", ""},
{"1.0stake", "", ""},
{"2.0stake,1.0trope", "1.9stake", "1.9stake"},
{"2.0stake,1.0trope", "2.1stake", "2.0stake"},
{"2.0stake,1.0trope", "0.9trope", "0.9trope"},
{"2.0stake,1.0trope", "1.9stake,0.9trope", "1.9stake,0.9trope"},
{"2.0stake,1.0trope", "1.9stake,0.9trope,20.0other", "1.9stake,0.9trope"},
{"2.0stake,1.0trope", "1.0other", ""},
}

for i, tc := range testCases {
in1, err := ParseDecCoins(tc.input1)
require.NoError(t, err, "unexpected parse error in %v", i)
in2, err := ParseDecCoins(tc.input2)
require.NoError(t, err, "unexpected parse error in %v", i)
exr, err := ParseDecCoins(tc.expectedResult)
require.NoError(t, err, "unexpected parse error in %v", i)

require.True(t, in1.Cap(in2).IsEqual(exr), "in1.cap(in2) != exr in %v", i)
// require.Equal(t, tc.expectedResult, in1.Cap(in2).String(), "in1.cap(in2) != exr in %v", i)
}
}
3 changes: 2 additions & 1 deletion x/distribution/keeper/allocation.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,11 @@ func (k Keeper) AllocateTokens(ctx sdk.Context, sumPrecommitPower, totalPower in
for _, vote := range votes {
validator := k.stakingKeeper.ValidatorByConsAddr(ctx, vote.Validator.Address)

// TODO likely we should only reward validators who actually signed the block.
// TODO consider microslashing for missing votes.
// ref https://github.com/cosmos/cosmos-sdk/issues/2525#issuecomment-430838701
powerFraction := sdk.NewDec(vote.Validator.Power).QuoTruncate(sdk.NewDec(totalPower))
reward := feesCollected.MulDecTruncate(voteMultiplier).MulDecTruncate(powerFraction)
reward = reward.Cap(remaining)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm is this the best way to fix? If this is necessary, it seems like our multiplication somewhere rounds up where it ought to always floor, so instead I wonder if we should change the multiplication (MulDec maybe) to always floor, or add a version that does and use it here.

k.AllocateTokensToValidator(ctx, validator, reward)
remaining = remaining.Sub(reward)
}
Expand Down