R4R F1 mechanism rounding fix

PENDING.md

@@ -23,6 +23,7 @@
 
 * [\#3669] Ensure consistency in message naming, codec registration, and JSON
 tags.
+* #3788 change order of operations for greater accuracy when calculation delegation share token value
 
 ### Tendermint
 

client/lcd/lcd_test.go

@@ -553,7 +553,7 @@ func TestBonding(t *testing.T) {
 	// hence we utilize the exchange rate in the following test
 
 	validator2 := getValidator(t, port, operAddrs[1])
-	delTokensAfterRedelegation := delegatorDels[0].GetShares().Mul(validator2.DelegatorShareExRate())
+	delTokensAfterRedelegation := validator2.ShareTokens(delegatorDels[0].GetShares())
 	require.Equal(t, rdTokens.ToDec(), delTokensAfterRedelegation)
 
 	redelegation := getRedelegations(t, port, addr, operAddrs[0], operAddrs[1])

types/dec_coin.go

@@ -478,6 +478,27 @@ func (coins DecCoins) IsAllPositive() bool {
 	return true
 }
 
+// MinSet will return a new set of coins which contains the minimum decCoin
+// for common denoms found in both `coins` and `coinsB`. For denoms not common
+// to both `coins` and `coinsB` the minimum is considered to be 0, thus they are
+// not added to the final set.
+func MinSet(coinsA, coinsB DecCoins) DecCoins {
+	minSet := ([]DecCoin)(nil)
+
+	for _, coinA := range coinsA {
+		denom := coinA.Denom
+		amountA := coinA.Amount
+		amountB := coinsB.AmountOf(coinA.Denom)
+
+		if amountA.LT(amountB) {
+			minSet = append(minSet, coinA)
+		} else {
+			minSet = append(minSet, NewDecCoinFromDec(denom, amountB))
+		}
+	}
+	return minSet
+}
+
 func removeZeroDecCoins(coins DecCoins) DecCoins {
 	i, l := 0, len(coins)
 	for i < l {

types/staking.go

@@ -61,20 +61,20 @@ func (b BondStatus) Equal(b2 BondStatus) bool {
 
 // validator for a delegated proof of stake system
 type Validator interface {
-	GetJailed() bool                       // whether the validator is jailed
-	GetMoniker() string                    // moniker of the validator
-	GetStatus() BondStatus                 // status of the validator
-	GetOperator() ValAddress               // operator address to receive/return validators coins
-	GetConsPubKey() crypto.PubKey          // validation consensus pubkey
-	GetConsAddr() ConsAddress              // validation consensus address
-	GetTokens() Int                        // validation tokens
-	GetBondedTokens() Int                  // validator bonded tokens
-	GetTendermintPower() int64             // validation power in tendermint
-	GetCommission() Dec                    // validator commission rate
-	GetMinSelfDelegation() Int             // validator minimum self delegation
-	GetDelegatorShares() Dec               // total outstanding delegator shares
-	GetDelegatorShareExRate() Dec          // tokens per delegator share exchange rate
-	GetDelegatorShareExRateTruncated() Dec // tokens per delegator share exchange rate
+	GetJailed() bool              // whether the validator is jailed
+	GetMoniker() string           // moniker of the validator
+	GetStatus() BondStatus        // status of the validator
+	GetOperator() ValAddress      // operator address to receive/return validators coins
+	GetConsPubKey() crypto.PubKey // validation consensus pubkey
+	GetConsAddr() ConsAddress     // validation consensus address
+	GetTokens() Int               // validation tokens
+	GetBondedTokens() Int         // validator bonded tokens
+	GetTendermintPower() int64    // validation power in tendermint
+	GetCommission() Dec           // validator commission rate
+	GetMinSelfDelegation() Int    // validator minimum self delegation
+	GetDelegatorShares() Dec      // total outstanding delegator shares
+	ShareTokens(Dec) Dec          // token worth of provided delegator shares
+	ShareTokensTruncated(Dec) Dec // token worth of provided delegator shares, truncated
 }
 
 // validator which fulfills abci validator interface for use in Tendermint

x/distribution/keeper/delegation.go

@@ -22,7 +22,7 @@ func (k Keeper) initializeDelegation(ctx sdk.Context, val sdk.ValAddress, del sd
 	// calculate delegation stake in tokens
 	// we don't store directly, so multiply delegation shares * (tokens per share)
 	// note: necessary to truncate so we don't allow withdrawing more rewards than owed
-	stake := delegation.GetShares().MulTruncate(validator.GetDelegatorShareExRateTruncated())
+	stake := validator.ShareTokensTruncated(delegation.GetShares())
 	k.SetDelegatorStartingInfo(ctx, val, del, types.NewDelegatorStartingInfo(previousPeriod, stake, uint64(ctx.BlockHeight())))
 }
 
@@ -90,15 +90,16 @@ func (k Keeper) calculateDelegationRewards(ctx sdk.Context, val sdk.Validator, d
 	// a stake sanity check - recalculated final stake should be less than or equal to current stake
 	// here we cannot use Equals because stake is truncated when multiplied by slash fractions
 	// we could only use equals if we had arbitrary-precision rationals
-	if stake.GT(del.GetShares().Mul(val.GetDelegatorShareExRate())) {
+	currentStake := val.ShareTokens(del.GetShares())
+	if stake.GT(currentStake) {
 		panic(fmt.Sprintf("calculated final stake for delegator %s greater than current stake: %s, %s",
-			del.GetDelegatorAddr(), stake, del.GetShares().Mul(val.GetDelegatorShareExRate())))
+			del.GetDelegatorAddr(), stake, currentStake))
 	}
 
 	// calculate rewards for final period
 	rewards = rewards.Add(k.calculateDelegationRewardsBetween(ctx, val, startingPeriod, endingPeriod, stake))
 
-	return
+	return rewards
 }
 
 func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val sdk.Validator, del sdk.Delegation) sdk.Error {
@@ -110,7 +111,19 @@ func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val sdk.Validator, de
 
 	// end current period and calculate rewards
 	endingPeriod := k.incrementValidatorPeriod(ctx, val)
-	rewards := k.calculateDelegationRewards(ctx, val, del, endingPeriod)
+	rewardsRaw := k.calculateDelegationRewards(ctx, val, del, endingPeriod)
+	outstanding := k.GetValidatorOutstandingRewards(ctx, del.GetValidatorAddr())
+
+	// defensive edge case may happen on the very final digits
+	// of the decCoins due to operation order of the distribution mechanism.
+	// TODO log if rewards is reduced in this step
+	rewards := sdk.MinSet(rewardsRaw, outstanding)
+	if !rewards.IsEqual(rewardsRaw) {
+		logger := ctx.Logger().With("module", "x/distr")
+		logger.Info(fmt.Sprintf("missing rewards rounding error, delegator %v"+
+			"withdrawing rewards from validator %v, should have received %v, got %v",
+			val.GetOperator(), del.GetDelegatorAddr(), rewardsRaw, rewards))
+	}
 
 	// decrement reference count of starting period
 	startingInfo := k.GetDelegatorStartingInfo(ctx, del.GetValidatorAddr(), del.GetDelegatorAddr())
@@ -120,7 +133,6 @@ func (k Keeper) withdrawDelegationRewards(ctx sdk.Context, val sdk.Validator, de
 	// truncate coins, return remainder to community pool
 	coins, remainder := rewards.TruncateDecimal()
 
-	outstanding := k.GetValidatorOutstandingRewards(ctx, del.GetValidatorAddr())
 	k.SetValidatorOutstandingRewards(ctx, del.GetValidatorAddr(), outstanding.Sub(rewards))
 	feePool := k.GetFeePool(ctx)
 	feePool.CommunityPool = feePool.CommunityPool.Add(remainder)

x/slashing/handler.go

@@ -31,7 +31,7 @@ func handleMsgUnjail(ctx sdk.Context, msg MsgUnjail, k Keeper) sdk.Result {
 		return ErrMissingSelfDelegation(k.codespace).Result()
 	}
 
-	if validator.GetDelegatorShareExRate().Mul(selfDel.GetShares()).TruncateInt().LT(validator.GetMinSelfDelegation()) {
+	if validator.ShareTokens(selfDel.GetShares()).TruncateInt().LT(validator.GetMinSelfDelegation()) {
 		return ErrSelfDelegationTooLowToUnjail(k.codespace).Result()
 	}
 

x/staking/handler_test.go

@@ -333,8 +333,6 @@ func TestIncrementsMsgDelegate(t *testing.T) {
 	require.Equal(t, bondAmount, bond.Shares.RoundInt())
 
 	pool := keeper.GetPool(ctx)
-	exRate := validator.DelegatorShareExRate()
-	require.True(t, exRate.Equal(sdk.OneDec()), "expected exRate 1 got %v", exRate)
 	require.Equal(t, bondAmount, pool.BondedTokens)
 
 	// just send the same msgbond multiple times
@@ -352,9 +350,6 @@ func TestIncrementsMsgDelegate(t *testing.T) {
 		bond, found := keeper.GetDelegation(ctx, delegatorAddr, validatorAddr)
 		require.True(t, found)
 
-		exRate := validator.DelegatorShareExRate()
-		require.True(t, exRate.Equal(sdk.OneDec()), "expected exRate 1 got %v, i = %v", exRate, i)
-
 		expBond := bondAmount.MulRaw(i + 1)
 		expDelegatorShares := bondAmount.MulRaw(i + 2) // (1 self delegation)
 		expDelegatorAcc := initBond.Sub(expBond)

x/staking/keeper/delegation.go

@@ -450,7 +450,7 @@ func (k Keeper) Delegate(ctx sdk.Context, delAddr sdk.AccAddress, bondAmt sdk.In
 	// In some situations, the exchange rate becomes invalid, e.g. if
 	// Validator loses all tokens due to slashing. In this case,
 	// make all future delegations invalid.
-	if validator.DelegatorShareExRate().IsZero() {
+	if validator.InvalidExRate() {
 		return sdk.ZeroDec(), types.ErrDelegatorShareExRateInvalid(k.Codespace())
 	}
 
@@ -517,7 +517,9 @@ func (k Keeper) unbond(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValA
 
 	// if the delegation is the operator of the validator and undelegating will decrease the validator's self delegation below their minimum
 	// trigger a jail validator
-	if isValidatorOperator && !validator.Jailed && validator.DelegatorShareExRate().Mul(delegation.Shares).TruncateInt().LT(validator.MinSelfDelegation) {
+	if isValidatorOperator && !validator.Jailed &&
+		validator.ShareTokens(delegation.Shares).TruncateInt().LT(validator.MinSelfDelegation) {
+
 		k.jailValidator(ctx, validator)
 		validator = k.mustGetValidator(ctx, validator.OperatorAddress)
 	}

x/staking/types/validator.go

@@ -348,18 +348,20 @@ func (v Validator) SetInitialCommission(commission Commission) (Validator, sdk.E
 // CONTRACT: Tokens are assumed to have come from not-bonded pool.
 func (v Validator) AddTokensFromDel(pool Pool, amount sdk.Int) (Validator, Pool, sdk.Dec) {
 
-	// bondedShare/delegatedShare
-	exRate := v.DelegatorShareExRate()
-	if exRate.IsZero() {
-		panic("zero exRate should not happen")
+	// calculate the shares to issue
+	var issuedShares sdk.Dec
+	if v.DelegatorShares.IsZero() {
+		// the first delegation to a validator sets the exchange rate to one
+		issuedShares = amount.ToDec()
+	} else {
+		issuedShares = v.DelegatorShares.MulInt(amount).QuoInt(v.Tokens)
 	}
 
 	if v.Status == sdk.Bonded {
 		pool = pool.notBondedTokensToBonded(amount)
 	}
 
 	v.Tokens = v.Tokens.Add(amount)
-	issuedShares := amount.ToDec().Quo(exRate)
 	v.DelegatorShares = v.DelegatorShares.Add(issuedShares)
 
 	return v, pool, issuedShares
@@ -382,7 +384,7 @@ func (v Validator) RemoveDelShares(pool Pool, delShares sdk.Dec) (Validator, Poo
 
 		// leave excess tokens in the validator
 		// however fully use all the delegator shares
-		issuedTokens = v.DelegatorShareExRate().Mul(delShares).TruncateInt()
+		issuedTokens = v.ShareTokens(delShares).TruncateInt()
 		v.Tokens = v.Tokens.Sub(issuedTokens)
 		if v.Tokens.IsNegative() {
 			panic("attempting to remove more tokens than available in validator")
@@ -397,24 +399,21 @@ func (v Validator) RemoveDelShares(pool Pool, delShares sdk.Dec) (Validator, Poo
 	return v, pool, issuedTokens
 }
 
-// DelegatorShareExRate gets the exchange rate of tokens over delegator shares.
-// UNITS: tokens/delegator-shares
-func (v Validator) DelegatorShareExRate() sdk.Dec {
-	if v.DelegatorShares.IsZero() {
-		// the first delegation to a validator sets the exchange rate to one
-		return sdk.OneDec()
-	}
-	return v.Tokens.ToDec().Quo(v.DelegatorShares)
+// In some situations, the exchange rate becomes invalid, e.g. if
+// Validator loses all tokens due to slashing. In this case,
+// make all future delegations invalid.
+func (v Validator) InvalidExRate() bool {
+	return v.Tokens.IsZero() && v.DelegatorShares.IsPositive()
 }
 
-// DelegatorShareExRateTruncated gets the exchange rate of tokens over delegator shares, truncated.
-// UNITS: tokens/delegator-shares
-func (v Validator) DelegatorShareExRateTruncated() sdk.Dec {
-	if v.DelegatorShares.IsZero() {
-		// the first delegation to a validator sets the exchange rate to one
-		return sdk.OneDec()
-	}
-	return v.Tokens.ToDec().QuoTruncate(v.DelegatorShares)
+// calculate the token worth of provided shares
+func (v Validator) ShareTokens(shares sdk.Dec) sdk.Dec {
+	return (shares.MulInt(v.Tokens)).Quo(v.DelegatorShares)
+}
+
+// calculate the token worth of provided shares, truncated
+func (v Validator) ShareTokensTruncated(shares sdk.Dec) sdk.Dec {
+	return (shares.MulInt(v.Tokens)).QuoTruncate(v.DelegatorShares)
 }
 
 // get the bonded tokens which the validator holds
@@ -443,19 +442,15 @@ func (v Validator) PotentialTendermintPower() int64 {
 var _ sdk.Validator = Validator{}
 
 // nolint - for sdk.Validator
-func (v Validator) GetJailed() bool                  { return v.Jailed }
-func (v Validator) GetMoniker() string               { return v.Description.Moniker }
-func (v Validator) GetStatus() sdk.BondStatus        { return v.Status }
-func (v Validator) GetOperator() sdk.ValAddress      { return v.OperatorAddress }
-func (v Validator) GetConsPubKey() crypto.PubKey     { return v.ConsPubKey }
-func (v Validator) GetConsAddr() sdk.ConsAddress     { return sdk.ConsAddress(v.ConsPubKey.Address()) }
-func (v Validator) GetTokens() sdk.Int               { return v.Tokens }
-func (v Validator) GetBondedTokens() sdk.Int         { return v.BondedTokens() }
-func (v Validator) GetTendermintPower() int64        { return v.TendermintPower() }
-func (v Validator) GetCommission() sdk.Dec           { return v.Commission.Rate }
-func (v Validator) GetMinSelfDelegation() sdk.Int    { return v.MinSelfDelegation }
-func (v Validator) GetDelegatorShares() sdk.Dec      { return v.DelegatorShares }
-func (v Validator) GetDelegatorShareExRate() sdk.Dec { return v.DelegatorShareExRate() }
-func (v Validator) GetDelegatorShareExRateTruncated() sdk.Dec {
-	return v.DelegatorShareExRateTruncated()
-}
+func (v Validator) GetJailed() bool               { return v.Jailed }
+func (v Validator) GetMoniker() string            { return v.Description.Moniker }
+func (v Validator) GetStatus() sdk.BondStatus     { return v.Status }
+func (v Validator) GetOperator() sdk.ValAddress   { return v.OperatorAddress }
+func (v Validator) GetConsPubKey() crypto.PubKey  { return v.ConsPubKey }
+func (v Validator) GetConsAddr() sdk.ConsAddress  { return sdk.ConsAddress(v.ConsPubKey.Address()) }
+func (v Validator) GetTokens() sdk.Int            { return v.Tokens }
+func (v Validator) GetBondedTokens() sdk.Int      { return v.BondedTokens() }
+func (v Validator) GetTendermintPower() int64     { return v.TendermintPower() }
+func (v Validator) GetCommission() sdk.Dec        { return v.Commission.Rate }
+func (v Validator) GetMinSelfDelegation() sdk.Int { return v.MinSelfDelegation }
+func (v Validator) GetDelegatorShares() sdk.Dec   { return v.DelegatorShares }