Skip to content

Commit

Permalink
fix(ERTP,vats): fix 9407 AmountPatternShape (#9863)
Browse files Browse the repository at this point in the history
Staged on #9410 

closes: #9407 
refs: #9410 

## Description

Originally #9410 was supposed to fix #9407 , by relaxing an incorrect rejection into a correct acceptance. But we are not yet ready to deploy such a fix on-chain, due to upgrade constraints, so fixing it in agoric-sdk master would be misleading. It might lead people to write other code that depends on this bug being fixed when tested against agoric-sdk master, only to have that code fail later on-chain when invoking the non-upgraded buggy code.

So we unfixed #9410 to restore the buggy behavior, but with a better error message, and refactored to make fixing it again easier. This PR is that fix. It will remain draft until we are confident we can upgrade the relevant on-chain vat (the vbank) in a timely manner. Nevertheless, though we'll keep it in draft until then as a safety measure, this PR is reviewable. Reviewers, please review it.

### Security Considerations
Fixing a bug is generally good for security. Otherwise, none.

### Scaling Considerations
none

### Documentation Considerations
Would remove the need to document the #9407 buggy behavior

### Testing Considerations
Restores the `test.failing` from #9410 to a `test`.

### Upgrade Considerations
as above.
  • Loading branch information
erights authored Nov 13, 2024
1 parent 21760b0 commit 59b1a9f
Show file tree
Hide file tree
Showing 2 changed files with 1 addition and 99 deletions.
20 changes: 1 addition & 19 deletions packages/vats/src/virtual-purse.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Fail } from '@endo/errors';
import { E } from '@endo/far';
import { isPromise } from '@endo/promise-kit';
import { getInterfaceGuardPayload, matches } from '@endo/patterns';
import { getInterfaceGuardPayload } from '@endo/patterns';

import { M } from '@agoric/store';
import {
Expand Down Expand Up @@ -105,22 +105,6 @@ export const makeVirtualPurseKitIKit = (
* current balance iterable for a given brand.
*/

/**
* Until https://github.com/Agoric/agoric-sdk/issues/9407 is fixed, this
* function restricts the `optAmountShape`, if provided, to be a concrete
* `Amount` rather than a `Pattern` as it is supposed to be.
*
* TODO: Once https://github.com/Agoric/agoric-sdk/issues/9407 is fixed, remove
* this function and all calls to it.
*
* @param {Pattern} [optAmountShape]
*/
const legacyRestrictAmountShapeArg = optAmountShape => {
if (optAmountShape && !matches(optAmountShape, AmountShape)) {
throw Fail`optAmountShape if provided, must still be a concrete Amount due to https://github.com/Agoric/agoric-sdk/issues/9407`;
}
};

/** @param {import('@agoric/zone').Zone} zone */
const prepareVirtualPurseKit = zone =>
zone.exoClassKit(
Expand Down Expand Up @@ -158,7 +142,6 @@ const prepareVirtualPurseKit = zone =>
* @returns {Promise<Payment<'nat'>>}
*/
async recoverableClaim(payment, optAmountShape) {
legacyRestrictAmountShapeArg(optAmountShape);
const {
state: { recoveryPurse },
} = this;
Expand Down Expand Up @@ -186,7 +169,6 @@ const prepareVirtualPurseKit = zone =>
minter: {
/** @type {Retain} */
async retain(payment, optAmountShape) {
legacyRestrictAmountShapeArg(optAmountShape);
!!this.state.mint || Fail`minter cannot retain without a mint.`;
return E(this.state.issuer).burn(payment, optAmountShape);
},
Expand Down
80 changes: 0 additions & 80 deletions packages/vats/test/vpurse.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -169,86 +169,6 @@ test('makeVirtualPurse', async t => {
);
};

expected.pushAmount(fungible837);
await E(vpurse)
.deposit(payment, fungible837)
.then(checkDeposit)
.then(performWithdrawal)
.then(checkWithdrawal);
});

// TODO Once https://github.com/Agoric/agoric-sdk/issues/9407 is fixed,
// This test should replace the similar one above.
test.failing('makeVirtualPurse with optAmountShape pattern', async t => {
t.plan(22);
const { baggage } = t.context;
const zone = makeDurableZone(baggage).subZone('makeVirtualPurse');

const { expected, balanceUpdater, issuer, mint, brand, vpurse } = setup(
t,
zone,
);

const payment = mint.mintPayment(AmountMath.make(brand, 837n));

const notifier = E(vpurse).getCurrentAmountNotifier();
let nextUpdateP = E(notifier).getUpdateSince();

const checkNotifier = async () => {
const { value: balance, updateCount } = await nextUpdateP;
t.assert(
AmountMath.isEqual(await E(vpurse).getCurrentAmount(), balance),
`the notifier balance is the same as the purse`,
);
nextUpdateP = E(notifier).getUpdateSince(updateCount);
};

balanceUpdater.updateState(AmountMath.makeEmpty(brand));
await checkNotifier();
t.assert(
AmountMath.isEqual(
await E(vpurse).getCurrentAmount(),
AmountMath.makeEmpty(brand),
),
`empty purse is empty`,
);
t.is(await E(vpurse).getAllegedBrand(), brand, `purse's brand is correct`);
const fungible837 = AmountMath.make(brand, 837n);

const checkDeposit = async newPurseBalance => {
t.assert(
AmountMath.isEqual(newPurseBalance, fungible837),
`the amount returned is the payment amount`,
);
await checkNotifier();
t.assert(
AmountMath.isEqual(await E(vpurse).getCurrentAmount(), fungible837),
`the new purse balance is the payment's old balance`,
);
};

const performWithdrawal = () => {
expected.pullAmount(fungible837);
return E(vpurse).withdraw(fungible837);
};

const checkWithdrawal = async newPayment => {
await issuer.getAmountOf(newPayment).then(amount => {
t.assert(
AmountMath.isEqual(amount, fungible837),
`the withdrawn payment has the right balance`,
);
});
await checkNotifier();
t.assert(
AmountMath.isEqual(
await E(vpurse).getCurrentAmount(),
AmountMath.makeEmpty(brand),
),
`the purse is empty again`,
);
};

expected.pushAmount(fungible837);
await E(vpurse)
.deposit(payment, M.and(fungible837))
Expand Down

0 comments on commit 59b1a9f

Please sign in to comment.