Merge pull request #5111 from Agoric/4887-lien-bridge

fix: avoid race by requesting lien change as a delta

golang/cosmos/x/lien/keeper/keeper.go

@@ -19,7 +19,7 @@ type Keeper interface {
 	GetLien(ctx sdk.Context, addr sdk.AccAddress) types.Lien
 	SetLien(ctx sdk.Context, addr sdk.AccAddress, lien types.Lien)
 	IterateLiens(ctx sdk.Context, cb func(addr sdk.AccAddress, lien types.Lien) bool)
-	UpdateLien(ctx sdk.Context, addr sdk.AccAddress, newCoin sdk.Coin) error
+	ChangeLien(ctx sdk.Context, addr sdk.AccAddress, denom string, delta sdk.Int) (sdk.Int, error)
 	GetAccountState(ctx sdk.Context, addr sdk.AccAddress) types.AccountState
 	BondDenom(ctx sdk.Context) string
 	GetAllBalances(ctx sdk.Context, addr sdk.AccAddress) sdk.Coins
@@ -103,30 +103,34 @@ func (lk keeperImpl) IterateLiens(ctx sdk.Context, cb func(addr sdk.AccAddress,
 	}
 }
 
-// UpdateLien changes the liened amount of a single denomination in the given account.
+// ChangeLien changes the liened amount of a single denomination in the given account.
 // Either the old or new amount of the denomination can be zero.
 // Liens can always be decreased, but to increase a lien, the new total amount must
 // be vested and either bonded (for the staking token) or in the bank (for other tokens).
 // The total lien can have several different denominations. Each is adjusted
 // independently.
-func (lk keeperImpl) UpdateLien(ctx sdk.Context, addr sdk.AccAddress, newCoin sdk.Coin) error {
+//
+// The delta is given as a raw Int instead of a Coin since it may be negative.
+func (lk keeperImpl) ChangeLien(ctx sdk.Context, addr sdk.AccAddress, denom string, delta sdk.Int) (sdk.Int, error) {
 	oldLien := lk.GetLien(ctx, addr)
 	oldCoins := oldLien.Coins
-	denom := newCoin.Denom
-	newAmount := newCoin.Amount
-	oldAmount := oldCoins.AmountOf(denom)
-	if newAmount.Equal(oldAmount) {
+	oldAmt := oldCoins.AmountOf(denom)
+	if delta.IsZero() {
 		// no-op, no need to do anything
-		return nil
+		return oldAmt, nil
 	}
-
-	newCoins := oldCoins.Sub(sdk.NewCoins(sdk.NewCoin(denom, oldAmount))).Add(newCoin)
+	newAmt := oldAmt.Add(delta)
+	if newAmt.IsNegative() {
+		return oldAmt, fmt.Errorf("lien delta of %s is larger than existing balance %s", delta, oldAmt)
+	}
+	newCoin := sdk.NewCoin(denom, newAmt)
+	newCoins := oldCoins.Sub(sdk.NewCoins(sdk.NewCoin(denom, oldAmt))).Add(newCoin)
 	newLien := types.Lien{
 		Coins:     newCoins,
 		Delegated: oldLien.Delegated,
 	}
 
-	if newAmount.GT(oldAmount) {
+	if delta.IsPositive() {
 		// See if it's okay to increase the lien.
 		// Lien can be increased if the new amount is vested,
 		// not already liened, and if it's the bond denom,
@@ -135,21 +139,20 @@ func (lk keeperImpl) UpdateLien(ctx sdk.Context, addr sdk.AccAddress, newCoin sd
 		if denom == lk.BondDenom(ctx) {
 			bonded := state.Bonded.AmountOf(denom)
 			unvested := state.Unvested.AmountOf(denom)
-			if !newAmount.Add(unvested).LTE(bonded) {
-				return fmt.Errorf("want to lien %s but only %s vested and bonded", newCoin, bonded.Sub(unvested))
+			if !newAmt.Add(unvested).LTE(bonded) {
+				return oldAmt, fmt.Errorf("want to lien %s but only %s vested and bonded", newCoin, bonded.Sub(unvested))
 			}
 			newDelegated := bonded.Add(state.Unbonding.AmountOf(denom))
 			newLien.Delegated = sdk.NewCoins(sdk.NewCoin(denom, newDelegated))
 		} else {
 			inBank := lk.bankKeeper.GetBalance(ctx, addr, denom)
-			if !newAmount.LTE(inBank.Amount) {
-				return fmt.Errorf("want to lien %s but only %s available", newCoin, inBank)
+			if !newAmt.LTE(inBank.Amount) {
+				return oldAmt, fmt.Errorf("want to lien %s but only %s available", newCoin, inBank)
 			}
 		}
-		// XXX check vested
 	}
 	lk.SetLien(ctx, addr, newLien)
-	return nil
+	return newAmt, nil
 }
 
 // GetAccountState retrieves the AccountState for addr.

golang/cosmos/x/lien/keeper/keeper_test.go

@@ -448,7 +448,7 @@ func TestVesting(t *testing.T) {
 	}
 }
 
-func TestUpdateLien(t *testing.T) {
+func TestChangeLien(t *testing.T) {
 	for _, tt := range []struct {
 		name     string
 		state    types.AccountState
@@ -533,15 +533,20 @@ func TestUpdateLien(t *testing.T) {
 				t.Fatalf("account state want %+v, got %+v", tt.state, gotState)
 			}
 			bondDenom := sk.BondDenom(ctx)
-			newLien := sdk.NewInt64Coin(bondDenom, tt.newLien)
-			err := lk.UpdateLien(ctx, addr2, newLien)
+			delta := tt.newLien - tt.state.Liened.AmountOf(bondDenom).Int64()
+			gotInt, err := lk.ChangeLien(ctx, addr2, bondDenom, sdk.NewInt(delta))
 			if err != nil {
 				if !tt.wantFail {
 					t.Errorf("Update lien failed: %v", err)
 				}
 			} else if tt.wantFail {
 				t.Errorf("Update lien succeeded, but wanted failure")
+			} else {
+				if !gotInt.Equal(sdk.NewInt(tt.newLien)) {
+					t.Errorf("want new lien balance %s, got %d", gotInt, tt.newLien)
+				}
 			}
+
 		})
 	}
 }

golang/cosmos/x/lien/lien.go

@@ -24,7 +24,7 @@ type portMessage struct {
 	Type       string   `json:"type"`
 	Address    string   `json:"address"`
 	Denom      string   `json:"denom"`
-	Amount     sdk.Int  `json:"amount"`
+	Delta      sdk.Int  `json:"delta"`
 	Validators []string `json:"validators"`
 	Delegators []string `json:"delegators"`
 }
@@ -63,7 +63,7 @@ func NewPortHandler(k Keeper) vm.PortHandler {
 const (
 	LIEN_GET_ACCOUNT_STATE = "LIEN_GET_ACCOUNT_STATE"
 	LIEN_GET_STAKING       = "LIEN_GET_STAKING"
-	LIEN_SET_LIENED        = "LIEN_SET_LIENED"
+	LIEN_CHANGE_LIENED     = "LIEN_CHANGE_LIENED"
 )
 
 // Receive implements the vm.PortHandler method.
@@ -83,8 +83,8 @@ func (ch portHandler) Receive(ctx *vm.ControllerContext, str string) (string, er
 	case LIEN_GET_STAKING:
 		return ch.handleGetStaking(ctx.Context, msg)
 
-	case LIEN_SET_LIENED:
-		return ch.handleSetLiened(ctx.Context, msg)
+	case LIEN_CHANGE_LIENED:
+		return ch.handleChangeLiened(ctx.Context, msg)
 	}
 	return "", fmt.Errorf("unrecognized type %s", msg.Type)
 }
@@ -179,9 +179,9 @@ func (ch portHandler) handleGetAccountState(ctx sdk.Context, msg portMessage) (s
 	return string(bz), nil
 }
 
-// handleSetLiened processes a LIEN_SET_LIENED message.
+// handleChangeLiened processes a LIEN_CHANGE_LIENED message.
 // See spec/02_messages.md for the messages and responses.
-func (ch portHandler) handleSetLiened(ctx sdk.Context, msg portMessage) (string, error) {
+func (ch portHandler) handleChangeLiened(ctx sdk.Context, msg portMessage) (string, error) {
 	addr, err := sdk.AccAddressFromBech32(msg.Address)
 	if err != nil {
 		return "", fmt.Errorf("cannot convert %s to address: %w", msg.Address, err)
@@ -190,10 +190,14 @@ func (ch portHandler) handleSetLiened(ctx sdk.Context, msg portMessage) (string,
 	if err = sdk.ValidateDenom(denom); err != nil {
 		return "", fmt.Errorf("invalid denom %s: %w", denom, err)
 	}
-	newCoin := sdk.NewCoin(denom, msg.Amount)
 
-	if err := ch.keeper.UpdateLien(ctx, addr, newCoin); err != nil {
-		return "false", nil
+	newAmt, err := ch.keeper.ChangeLien(ctx, addr, denom, msg.Delta)
+	if err != nil {
+		return "", err
 	}
-	return "true", nil
+	bz, err := json.Marshal(&newAmt)
+	if err != nil {
+		return "", fmt.Errorf("cannot marshal %v: %w", newAmt, err)
+	}
+	return string(bz), nil
 }

golang/cosmos/x/lien/lien_test.go

@@ -73,10 +73,13 @@ func (m *mockLienKeeper) SetLien(ctx sdk.Context, addr sdk.AccAddress, lien type
 func (m *mockLienKeeper) IterateLiens(ctx sdk.Context, cb func(addr sdk.AccAddress, lien types.Lien) bool) {
 }
 
-func (m *mockLienKeeper) UpdateLien(ctx sdk.Context, addr sdk.AccAddress, newLien sdk.Coin) error {
+func (m *mockLienKeeper) ChangeLien(ctx sdk.Context, addr sdk.AccAddress, denom string, delta sdk.Int) (sdk.Int, error) {
+	state := m.GetAccountState(ctx, addr)
+	oldLiened := state.Liened.AmountOf(denom)
+	newLiened := oldLiened.Add(delta)
 	m.updateAddr = addr
-	m.updateCoin = newLien
-	return nil
+	m.updateCoin = sdk.NewCoin(denom, newLiened)
+	return newLiened, nil
 }
 
 func (m *mockLienKeeper) GetAccountState(ctx sdk.Context, addr sdk.AccAddress) types.AccountState {
@@ -216,10 +219,10 @@ func TestSetLiened_badAddr(t *testing.T) {
 	keeper := mockLienKeeper{}
 	ph := NewPortHandler(&keeper)
 	msg := portMessage{
-		Type:    "LIEN_SET_LIENED",
+		Type:    "LIEN_CHANGE_LIENED",
 		Address: "foo",
 		Denom:   "ubld",
-		Amount:  i(123),
+		Delta:   i(123),
 	}
 	jsonMsg, err := json.Marshal(&msg)
 	if err != nil {
@@ -237,10 +240,10 @@ func TestSetLiened_badDenom(t *testing.T) {
 	keeper := mockLienKeeper{}
 	ph := NewPortHandler(&keeper)
 	msg := portMessage{
-		Type:    "LIEN_SET_LIENED",
+		Type:    "LIEN_CHANGE_LIENED",
 		Address: addr1,
 		Denom:   "x",
-		Amount:  i(123),
+		Delta:   i(123),
 	}
 	jsonMsg, err := json.Marshal(&msg)
 	if err != nil {
@@ -258,10 +261,10 @@ func TestSetLiened(t *testing.T) {
 	keeper := mockLienKeeper{}
 	ph := NewPortHandler(&keeper)
 	msg := portMessage{
-		Type:    "LIEN_SET_LIENED",
+		Type:    "LIEN_CHANGE_LIENED",
 		Address: addr1,
 		Denom:   "ubld",
-		Amount:  i(123),
+		Delta:   i(123),
 	}
 	jsonMsg, err := json.Marshal(&msg)
 	if err != nil {
@@ -271,8 +274,8 @@ func TestSetLiened(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Receive error %v", err)
 	}
-	if reply != "true" {
-		t.Fatalf("Receive returned %s, want true", reply)
+	if reply != `"123"` {
+		t.Fatalf(`Receive returned %s, want "123"`, reply)
 	}
 	if keeper.updateAddr.String() != addr1 {
 		t.Errorf("lien update with address %s, want %s", keeper.updateAddr, addr1)

golang/cosmos/x/lien/spec/02_messages.md

@@ -8,27 +8,33 @@ This documents the messages sent across the bridge to the Javascript swingset.
 
 ## From Javascript to Golang
 
-### LIEN_SET_LIENED
+### LIEN_CHANGE_LIENED
 
-* type: "LIEN_SET_LIENED"
+Request: JSON object with the fields
+
+* type: "LIEN_CHANGE_LIENED"
 * address: string, bech32-encoded
 * denom: string
-* amount: string encoding nonnegative integer
+* delta: string encoding integer of change to liened amount
+
+Response: JSON string encoding nonnegative integer holding the current lien amount.
 
-Response: boolean indicating whether the total lien for the account
-successfully set to the new total. The following rules are used:
+The following rules are used:
 
 * The total liened amount can always be decreased.
-* When increasing the total liened amount, the new total must be less than
-  or equal to the bonded amount.
+* When increasing the total liened amount, the new total liened plus unvested
+  must be less than or equal to the bonded amount.
+* Can be used to change from zero balance or to zero balance.
 
 ### LIEN_GET_ACCOUNT_STATE
 
+Request: JSON object with the fields
+
 * type: "LIEN_GET_ACCOUNT_STATE"
 * address: string, bech32-encoded
 * denom: string
 
-Response:
+Response: JSON object with the fields
 
 * currentTime: string timestamp
 * total: string encoding nonnegativeinteger
@@ -41,11 +47,13 @@ See [Concepts](01_concepts.md) for the meaning of the amounts.
 
 ### LIEN_GET_STAKING
 
+Request: JSON object with the fields
+
 * type: "LIEN_GET_STAKING"
 * validators: array of strings of bech32-encoded operator addresses
 * delegators: array of strings of bech32-encoded account addresses
 
-Response:
+Response: JSON object with the fields
 
 * epoch_tag: string encoding the staking epoch.
 If it is the same in two different responses, the results can be
@@ -81,4 +89,8 @@ None.
 The LIEN_SET_TOTAL operation checks the Golang-side state and might reject
 the operation. It has the power to do so to avoid a race between checking
 the account state and updating the liened amount, vs other cosmos-sdk
-operations which might change the account state.
+operations which might change the account state. The cosmos-side lien amount
+is the source of truth for the lien.
+
+The LIEN_GET_STAKING call is categorized as a lien operation onlyb because
+of the similarity to LIEN_GET_ACCOUNT_STATE.

packages/run-protocol/src/my-lien.js

@@ -11,8 +11,8 @@ const { details: X } = assert;
  */
 const XLien = /** @type { const } */ ({
   name: 'lien',
+  LIEN_CHANGE_LIENED: 'LIEN_CHANGE_LIENED',
   LIEN_GET_ACCOUNT_STATE: 'LIEN_GET_ACCOUNT_STATE',
-  LIEN_SET_LIENED: 'LIEN_SET_LIENED',
 });
 
 /**
@@ -22,21 +22,44 @@ const XLien = /** @type { const } */ ({
 
 /**
  * @param {ERef<BridgeManager>} bridgeManager
- * @param {Brand<'nat'>} stake
+ * @param {Brand<'nat'>} brand
  * @param {string} [denom]
  * @returns {StakingAuthority}
  */
-export const makeStakeReporter = (bridgeManager, stake, denom = 'ubld') => {
+export const makeStakeReporter = (bridgeManager, brand, denom = 'ubld') => {
   const { make: makeAmt } = AmountMath;
   /** @param {string} numeral */
-  const toStake = numeral => makeAmt(stake, BigInt(numeral));
+  const toStake = numeral => makeAmt(brand, BigInt(numeral));
+  /**
+   * @param {string} address
+   * @param {bigint} delta
+   * @returns {Promise<Amount<`nat`>>}
+   */
+  const changeLiened = async (address, delta) => {
+    assert.typeof(address, 'string');
+    const newAmount = await E(bridgeManager).toBridge(XLien.name, {
+      type: XLien.LIEN_CHANGE_LIENED,
+      address,
+      denom,
+      delta: `${delta}`,
+    });
+    return harden(toStake(newAmount));
+  };
 
   /** @type {StakingAuthority} */
   const stakeReporter = Far('stakeReporter', {
+    increaseLiened: async (address, increase) => {
+      const delta = AmountMath.getValue(brand, increase);
+      return changeLiened(address, delta);
+    },
+    decreaseLiened: async (address, decrease) => {
+      const delta = -1n * AmountMath.getValue(brand, decrease);
+      return changeLiened(address, delta);
+    },
     getAccountState: async (address, wantedBrand) => {
       assert(
-        wantedBrand === stake,
-        X`Cannot getAccountState for ${wantedBrand}. Expected ${stake}.`,
+        wantedBrand === brand,
+        X`Cannot getAccountState for ${wantedBrand}. Expected ${brand}.`,
       );
       /** @type { AccountState<string> } */
       const { currentTime, bonded, liened, locked, total, unbonding } = await E(
@@ -56,18 +79,6 @@ export const makeStakeReporter = (bridgeManager, stake, denom = 'ubld') => {
         currentTime: BigInt(currentTime),
       });
     },
-    setLiened: async (address, previous, target) => {
-      assert.typeof(address, 'string');
-      AmountMath.coerce(stake, previous); // TODO
-      const amount = AmountMath.getValue(stake, target);
-      const success = await E(bridgeManager).toBridge(XLien.name, {
-        type: XLien.LIEN_SET_LIENED,
-        address,
-        denom,
-        amount: `${amount}`,
-      });
-      assert(success);
-    },
   });
 
   return stakeReporter;

packages/run-protocol/src/runStake/README.md

@@ -40,8 +40,8 @@ sequenceDiagram
     dapp ->> walletBridge: getReturnableAttestation(want: 450 RUN, give: 500 BLD-Att)
     note right of walletBridge: Blocks on user approval in wallet
     walletBridge ->> attestation: makeAttestation(500 BLD)
-    attestation ->> Cosmos_SDK: setLienedAmount(4000 + 500 BLD)
-    Cosmos_SDK -->> attestation: ACK or throws
+    attestation ->> Cosmos_SDK: increaseLiened(+500 BLD)
+    Cosmos_SDK -->> attestation: new lien balance or throws
 
     attestation -->> walletBridge: Payment of 500 BLD-Att liened on ag123