{Add,Reclaim}EscrowEvent: include share amounts

.changelog/3939.breaking.md

@@ -0,0 +1 @@
+staking: Include share amounts in `AddEscrow` and `ReclaimEscrow` events

.changelog/3939.bugfix.md

@@ -0,0 +1,13 @@
+Update emitted events when disbursing rewards
+
+Before, a single `AddEscrow` event was emitted which did not correctly represent
+the actual state changes when rewards were disbursed.
+
+Now the following events are emitted:
+
+- `Transfer(CommonPool -> Recipient, commissionAmount)` event for the
+  commissioned part of the reward
+- `AddEscrow(Recipient -> Recipient, commissionAmount)` event for the
+  automatically escrowed commission reward
+- `AddEscrow(CommonPool -> Recipient, restAmount)` for the non-commissioned part
+  of the reward (which only increases existing shares prices)

docs/consensus/staking.md

@@ -617,6 +617,7 @@ type AddEscrowEvent struct {
   Owner  Address           `json:"owner"`
   Escrow Address           `json:"escrow"`
   Amount quantity.Quantity `json:"amount"`
+  NewShares quantity.Quantity `json:"new_shares"`
 }
 ```
 
@@ -626,6 +627,9 @@ type AddEscrowEvent struct {
 * `escrow` contains the address of the destination account the tokens are being
   escrowed to.
 * `amount` contains the amount (in base units) escrowed.
+* `new_shares` contains the amount of shares created as a result of the added 
+escrow event. Can be zero in case of (non-commissioned) rewards, where stake is
+added without new shares to increase share price.
 
 #### Take Escrow Event
 
@@ -658,6 +662,7 @@ type ReclaimEscrowEvent struct {
   Owner  Address           `json:"owner"`
   Escrow Address           `json:"escrow"`
   Amount quantity.Quantity `json:"amount"`
+  Shares quantity.Quantity `json"shares"`
 }
 ```
 
@@ -666,6 +671,7 @@ type ReclaimEscrowEvent struct {
 * `owner` contains the address of the account that reclaimed tokens from escrow.
 * `escrow` contains the address of the account escrow has been reclaimed from.
 * `amount` contains the amount (in base units) reclaimed.
+* `shares` contains the amount of shares reclaimed.
 
 ### Allowance Change Event
 

go/consensus/tendermint/apps/staking/staking.go

@@ -272,12 +272,14 @@ func (app *stakingApplication) onEpochChange(ctx *api.Context, epoch beacon.Epoc
 			"escrow_addr", e.EscrowAddr,
 			"delegator_addr", e.DelegatorAddr,
 			"base_units", stakeAmount,
+			"num_shares", shareAmount,
 		)
 
 		evt := staking.ReclaimEscrowEvent{
 			Owner:  e.DelegatorAddr,
 			Escrow: e.EscrowAddr,
 			Amount: *stakeAmount,
+			Shares: *shareAmount,
 		}
 		ctx.EmitEvent(api.NewEventBuilder(app.Name()).Attribute(KeyReclaimEscrow, cbor.Marshal(evt)))
 	}

go/consensus/tendermint/apps/staking/state/state.go

@@ -822,8 +822,9 @@ func (s *MutableState) TransferFromCommon(
 		return false, nil
 	}
 
-	// If escrow is requested, escrow the transferred stake immediately.
-	if escrow {
+	switch escrow {
+	case true:
+		// If escrow is requested, escrow the transferred stake immediately.
 		var com *quantity.Quantity
 		switch to.Escrow.Active.TotalShares.IsZero() {
 		case false:
@@ -834,10 +835,9 @@ func (s *MutableState) TransferFromCommon(
 				return false, fmt.Errorf("tendermint/staking: failed to get current epoch: %w", err)
 			}
 
-			q := transferred.Clone()
 			rate := to.Escrow.CommissionSchedule.CurrentRate(epoch)
 			if rate != nil {
-				com = q.Clone()
+				com = transferred.Clone()
 				// Multiply first.
 				if err = com.Mul(rate); err != nil {
 					return false, fmt.Errorf("tendermint/staking: failed multiplying by commission rate: %w", err)
@@ -846,15 +846,27 @@ func (s *MutableState) TransferFromCommon(
 					return false, fmt.Errorf("tendermint/staking: failed dividing by commission rate denominator: %w", err)
 				}
 
-				if err = q.Sub(com); err != nil {
+				if err = transferred.Sub(com); err != nil {
 					return false, fmt.Errorf("tendermint/staking: failed subtracting commission: %w", err)
 				}
 			}
 
 			// Escrow everything except the commission (increases value of all shares).
-			if err = quantity.Move(&to.Escrow.Active.Balance, &to.General.Balance, q); err != nil {
+			if err = quantity.Move(&to.Escrow.Active.Balance, &to.General.Balance, transferred); err != nil {
 				return false, fmt.Errorf("tendermint/staking: failed transferring to active escrow balance from common pool: %w", err)
 			}
+
+			// Emit non commissioned reward event.
+			if !ctx.IsCheckOnly() {
+				ev := cbor.Marshal(&staking.AddEscrowEvent{
+					Owner:  staking.CommonPoolAddress,
+					Escrow: toAddr,
+					Amount: *transferred,
+					// No new shares as this is the reward. As a result existing share price increases.
+					NewShares: quantity.Quantity{},
+				})
+				ctx.EmitEvent(api.NewEventBuilder(AppName).Attribute(KeyAddEscrow, ev))
+			}
 		case true:
 			// If nothing has been escrowed before, everything counts as commission.
 			com = transferred.Clone()
@@ -868,41 +880,52 @@ func (s *MutableState) TransferFromCommon(
 				return false, fmt.Errorf("tendermint/staking: failed to query delegation: %w", err)
 			}
 
-			if err = to.Escrow.Active.Deposit(&delegation.Shares, &to.General.Balance, com); err != nil {
+			var obtainedShares *quantity.Quantity
+			obtainedShares, err = to.Escrow.Active.Deposit(&delegation.Shares, &to.General.Balance, com)
+			if err != nil {
 				return false, fmt.Errorf("tendermint/staking: failed to deposit to escrow: %w", err)
 			}
 
 			if err = s.SetDelegation(ctx, toAddr, toAddr, delegation); err != nil {
 				return false, fmt.Errorf("tendermint/staking: failed to set delegation: %w", err)
 			}
-		}
-	}
-
-	if err = s.SetCommonPool(ctx, commonPool); err != nil {
-		return false, fmt.Errorf("tendermint/staking: failed to set common pool: %w", err)
-	}
-	if err = s.SetAccount(ctx, toAddr, to); err != nil {
-		return false, fmt.Errorf("tendermint/staking: failed to set account %s: %w", toAddr, err)
-	}
 
-	// Emit event(s).
-	if !ctx.IsCheckOnly() {
-		switch escrow {
-		case false:
+			// Emit events.
+			// Commission was transferred to the account, and automatically escrowed.
+			if !ctx.IsCheckOnly() {
+				ev := cbor.Marshal(&staking.TransferEvent{
+					From:   staking.CommonPoolAddress,
+					To:     toAddr,
+					Amount: *com,
+				})
+				ctx.EmitEvent(api.NewEventBuilder(AppName).Attribute(KeyTransfer, ev))
+
+				ev = cbor.Marshal(&staking.AddEscrowEvent{
+					Owner:     toAddr,
+					Escrow:    toAddr,
+					Amount:    *com,
+					NewShares: *obtainedShares,
+				})
+				ctx.EmitEvent(api.NewEventBuilder(AppName).Attribute(KeyAddEscrow, ev))
+			}
+		}
+	case false:
+		if !ctx.IsCheckOnly() {
 			ev := cbor.Marshal(&staking.TransferEvent{
 				From:   staking.CommonPoolAddress,
 				To:     toAddr,
 				Amount: *transferred,
 			})
 			ctx.EmitEvent(api.NewEventBuilder(AppName).Attribute(KeyTransfer, ev))
-		case true:
-			ev := cbor.Marshal(&staking.AddEscrowEvent{
-				Owner:  staking.CommonPoolAddress,
-				Escrow: toAddr,
-				Amount: *transferred,
-			})
-			ctx.EmitEvent(api.NewEventBuilder(AppName).Attribute(KeyAddEscrow, ev))
 		}
+
+	}
+
+	if err = s.SetCommonPool(ctx, commonPool); err != nil {
+		return false, fmt.Errorf("tendermint/staking: failed to set common pool: %w", err)
+	}
+	if err = s.SetAccount(ctx, toAddr, to); err != nil {
+		return false, fmt.Errorf("tendermint/staking: failed to set account %s: %w", toAddr, err)
 	}
 
 	return true, nil
@@ -1108,6 +1131,8 @@ func (s *MutableState) AddRewards(
 				Owner:  staking.CommonPoolAddress,
 				Escrow: addr,
 				Amount: *q,
+				// No new shares as this is the reward. As a result existing share price increases.
+				NewShares: quantity.Quantity{},
 			})
 			ctx.EmitEvent(api.NewEventBuilder(AppName).Attribute(KeyAddEscrow, ev))
 		}
@@ -1119,20 +1144,33 @@ func (s *MutableState) AddRewards(
 				return fmt.Errorf("tendermint/staking: failed to query delegation: %w", err)
 			}
 
-			if err = ent.Escrow.Active.Deposit(&delegation.Shares, commonPool, com); err != nil {
+			var obtainedShares *quantity.Quantity
+			obtainedShares, err = ent.Escrow.Active.Deposit(&delegation.Shares, commonPool, com)
+			if err != nil {
 				return fmt.Errorf("tendermint/staking: depositing commission: %w", err)
 			}
 
 			if err = s.SetDelegation(ctx, addr, addr, delegation); err != nil {
 				return fmt.Errorf("tendermint/staking: failed to set delegation: %w", err)
 			}
 
-			ev := cbor.Marshal(&staking.AddEscrowEvent{
-				Owner:  staking.CommonPoolAddress,
-				Escrow: addr,
+			// Above, we directly desposit from the common pool into the delegation,
+			// which is a shorthand for transferring to the account and immediately
+			// escrowing it. Explicitly emit both events.
+			ev := cbor.Marshal(&staking.TransferEvent{
+				From:   staking.CommonPoolAddress,
+				To:     addr,
 				Amount: *com,
 			})
 			ctx.EmitEvent(api.NewEventBuilder(AppName).Attribute(KeyAddEscrow, ev))
+
+			ev = cbor.Marshal(&staking.AddEscrowEvent{
+				Owner:     addr,
+				Escrow:    addr,
+				Amount:    *com,
+				NewShares: *obtainedShares,
+			})
+			ctx.EmitEvent(api.NewEventBuilder(AppName).Attribute(KeyAddEscrow, ev))
 		}
 
 		if err = s.SetAccount(ctx, addr, ent); err != nil {
@@ -1236,6 +1274,8 @@ func (s *MutableState) AddRewardSingleAttenuated(
 			Owner:  staking.CommonPoolAddress,
 			Escrow: address,
 			Amount: *q,
+			// No new shares as this is the reward. As a result existing share price increases.
+			NewShares: quantity.Quantity{},
 		})
 		ctx.EmitEvent(api.NewEventBuilder(AppName).Attribute(KeyAddEscrow, ev))
 	}
@@ -1247,20 +1287,33 @@ func (s *MutableState) AddRewardSingleAttenuated(
 			return fmt.Errorf("tendermint/staking: failed to query delegation: %w", err)
 		}
 
-		if err = acct.Escrow.Active.Deposit(&delegation.Shares, commonPool, com); err != nil {
+		var obtainedShares *quantity.Quantity
+		obtainedShares, err = acct.Escrow.Active.Deposit(&delegation.Shares, commonPool, com)
+		if err != nil {
 			return fmt.Errorf("tendermint/staking: failed depositing commission: %w", err)
 		}
 
 		if err = s.SetDelegation(ctx, address, address, delegation); err != nil {
 			return fmt.Errorf("tendermint/staking: failed to set delegation: %w", err)
 		}
 
-		ev := cbor.Marshal(&staking.AddEscrowEvent{
-			Owner:  staking.CommonPoolAddress,
-			Escrow: address,
+		// Above, we directly desposit from the common pool into the delegation,
+		// which is a shorthand for transferring to the account and immediately
+		// escrowing it. Explicitly emit both events.
+		ev := cbor.Marshal(&staking.TransferEvent{
+			From:   staking.CommonPoolAddress,
+			To:     address,
 			Amount: *com,
 		})
 		ctx.EmitEvent(api.NewEventBuilder(AppName).Attribute(KeyAddEscrow, ev))
+
+		ev = cbor.Marshal(&staking.AddEscrowEvent{
+			Owner:     address,
+			Escrow:    address,
+			Amount:    *com,
+			NewShares: *obtainedShares,
+		})
+		ctx.EmitEvent(api.NewEventBuilder(AppName).Attribute(KeyAddEscrow, ev))
 	}
 
 	if err = s.SetAccount(ctx, address, acct); err != nil {

go/consensus/tendermint/apps/staking/state/state_test.go

@@ -73,15 +73,18 @@ func TestDelegationQueries(t *testing.T) {
 
 		// Init delegation.
 		var del staking.Delegation
-		err = escrowAccount.Escrow.Active.Deposit(&del.Shares, &account.General.Balance, mustInitQuantityP(t, i*100))
+		var newShares *quantity.Quantity
+		newShares, err = escrowAccount.Escrow.Active.Deposit(&del.Shares, &account.General.Balance, mustInitQuantityP(t, i*100))
 		require.NoError(err, "active escrow deposit")
+		require.Equal(newShares, &del.Shares, "new shares should equal initial delegation")
 		expectedDelegations[escrowAddr][addr] = &del
 
 		// Init debonding delegation.
 		var deb staking.DebondingDelegation
 		deb.DebondEndTime = beacon.EpochTime(i)
-		err = escrowAccount.Escrow.Debonding.Deposit(&deb.Shares, &account.General.Balance, mustInitQuantityP(t, i*100))
+		newShares, err = escrowAccount.Escrow.Debonding.Deposit(&deb.Shares, &account.General.Balance, mustInitQuantityP(t, i*100))
 		require.NoError(err, "debonding escrow deposit")
+		require.Equal(newShares, &del.Shares, "new shares should equal initial debonding delegation")
 		expectedDebDelegations[escrowAddr][addr] = []*staking.DebondingDelegation{&deb}
 
 		// Update state.
@@ -223,12 +226,12 @@ func TestRewardAndSlash(t *testing.T) {
 	require.NoError(err, "commission schedule")
 
 	del := &staking.Delegation{}
-	err = escrowAccount.Escrow.Active.Deposit(&del.Shares, &delegatorAccount.General.Balance, mustInitQuantityP(t, 100))
+	_, err = escrowAccount.Escrow.Active.Deposit(&del.Shares, &delegatorAccount.General.Balance, mustInitQuantityP(t, 100))
 	require.NoError(err, "active escrow deposit")
 
 	var deb staking.DebondingDelegation
 	deb.DebondEndTime = 21
-	err = escrowAccount.Escrow.Debonding.Deposit(&deb.Shares, &delegatorAccount.General.Balance, mustInitQuantityP(t, 100))
+	_, err = escrowAccount.Escrow.Debonding.Deposit(&deb.Shares, &delegatorAccount.General.Balance, mustInitQuantityP(t, 100))
 	require.NoError(err, "debonding escrow deposit")
 
 	now := time.Unix(1580461674, 0)
@@ -554,7 +557,7 @@ func TestTransferFromCommon(t *testing.T) {
 
 	// Transfer with escrow (other delegations and commission).
 	var dg2 staking.Delegation
-	err = acc2.Escrow.Active.Deposit(&dg2.Shares, quantity.NewFromUint64(100), quantity.NewFromUint64(100))
+	_, err = acc2.Escrow.Active.Deposit(&dg2.Shares, quantity.NewFromUint64(100), quantity.NewFromUint64(100))
 	require.NoError(err, "Deposit")
 	err = s.SetDelegation(ctx, addr1, addr2, &dg2)
 	require.NoError(err, "SetDelegation")

go/consensus/tendermint/apps/staking/transactions.go

@@ -233,7 +233,8 @@ func (app *stakingApplication) addEscrow(ctx *api.Context, state *stakingState.M
 		return fmt.Errorf("failed to fetch delegation: %w", err)
 	}
 
-	if err = to.Escrow.Active.Deposit(&delegation.Shares, &from.General.Balance, &escrow.Amount); err != nil {
+	obtainedShares, err := to.Escrow.Active.Deposit(&delegation.Shares, &from.General.Balance, &escrow.Amount)
+	if err != nil {
 		ctx.Logger().Error("AddEscrow: failed to escrow stake",
 			"err", err,
 			"from", fromAddr,
@@ -261,12 +262,14 @@ func (app *stakingApplication) addEscrow(ctx *api.Context, state *stakingState.M
 		"from", fromAddr,
 		"to", escrow.Account,
 		"amount", escrow.Amount,
+		"obtained_shares", obtainedShares,
 	)
 
 	evt := &staking.AddEscrowEvent{
-		Owner:  fromAddr,
-		Escrow: escrow.Account,
-		Amount: escrow.Amount,
+		Owner:     fromAddr,
+		Escrow:    escrow.Account,
+		Amount:    escrow.Amount,
+		NewShares: *obtainedShares,
 	}
 	ctx.EmitEvent(api.NewEventBuilder(app.Name()).Attribute(KeyAddEscrow, cbor.Marshal(evt)))
 
@@ -365,7 +368,7 @@ func (app *stakingApplication) reclaimEscrow(ctx *api.Context, state *stakingSta
 	}
 	stakeAmount := baseUnits.Clone()
 
-	if err = from.Escrow.Debonding.Deposit(&deb.Shares, &baseUnits, stakeAmount); err != nil {
+	if _, err = from.Escrow.Debonding.Deposit(&deb.Shares, &baseUnits, stakeAmount); err != nil {
 		ctx.Logger().Error("ReclaimEscrow: failed to debond shares",
 			"err", err,
 			"to", toAddr,