faulty amountShape param in interface guard

[turadg]

Describe the bug

Some ERTP methods have an optAmountShape param that ends up here

agoric-sdk/packages/ERTP/src/paymentLedger.js

Lines 342 to 350 in e40225e

	/**
	* @param {Payment} payment awaited by callWhen
	* @param {Pattern} optAmountShape
	*/
	burn(payment, optAmountShape = undefined) {
	// IssuerI delays calling this method until `payment` is a Remotable
	assertLivePayment(payment);
	const paymentBalance = paymentLedger.get(payment);
	assertAmountConsistent(paymentBalance, optAmountShape);

That confirms that the amount matches the shape.

Some interface guards use that shape object directly, but they are shape specifiers themselves so it has to be something like AmountShapeShape. (or simply M.pattern())

What happens now when using that is an error

To Reproduce

Steps to reproduce the behavior:

1. Deposit to a VirtualPurse with an amountShape param
2. Get this error:

In "retain" method of (VirtualPurseKit utils): arg 1?: value: "[match:recordOf]" - Must match one of ["[match:nat]","[match:setOf]","[match:arrayOf]","[match:bagOf]"]

Expected behavior

No error.

Additional context

#9402 has a fix in 0ce647f but it requires a change to vat-bank which is in production.

Some options:

1. document the frailty and recommend not using it, but leave the production behavior
2. upgrade vat-bank with the fix
3. fix it in master but leave production alone
4. update master to remove the option so that it's an error to try to use it (because it will fail in production anyway)

[Chris-Hibbert]

I don't get some of the details.

• Is anything wrong with non-virtual purses? Are they incompatible with virtual purses?
• does "recommend not using it" apply to some parameters of non-virtual purse (or other objects)?
• are you recommending any changes in /packages/ERTP/?
• How hard would it be to upgrade vat-bank? I see that it's not a contract. Is it durable? It makes use of baggage. Has upgrade been tested?

[turadg]

Is anything wrong with non-virtual purses?

No, it uses M.pattern() for the optAmountShape param.

Are they incompatible with virtual purses?

They have different uses. If you are asking whether their interfaces differ, vpurses have more methods and guards.

does "recommend not using it" apply to some parameters of non-virtual purse (or other objects)?

I'm only talking about virtual purses. The idea is to add a JSDoc to the param explaining that it's broken and not to pass it in. The TypeScript could type the param as never to raise an error if it's tried.

are you recommending any changes in /packages/ERTP/?

The fix would go in virtual-purse.js which is in vats package. One idea is to use an AmountShapeShape which I propose would live in ERTP's typeGuards.js

/** For parameters that are themselves an AmountShape pattern. */
export const AmountShapeShape = harden({
  brand: M.pattern(),
  value: M.pattern(),
});

How hard would it be to upgrade vat-bank? I see that it's not a contract. Is it durable? It makes use of baggage. Has upgrade been tested?

Here is a test of upgrade:

agoric-sdk/packages/boot/test/upgrading/upgrade-vats.test.js

Line 231 in 50f9493

test('upgrade vat-bank', async t => {

[dckc]

To Reproduce

Steps to reproduce the behavior:

1. Deposit to a VirtualPurse with an amountShape param

not just any amountShape, right? an exact amount such as AmountMath.make(brand1, 123n) would not trigger the bug, would it? only something like harden({ brand1, M.gte(10n)}) that doesn't match AmountShape would trigger it, right?

[turadg]

Correct, if you give a concrete value with no matchers, it will work. That's not what the API describes though.

[erights]

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. #9863 is that fix. #9863 will remain draft until we are confident we can upgrade the relevant on-chain vat (the vbank) in a timely manner. Nevertheless, we'll keep #9863 in draft until then as a safety measure.

[erights]

Nevertheless, we'll keep #9863 in draft until then as a safety measure.

I marked #9863 Ready for Review. PTAL there.