9207 fake bank #9402

turadg · 2024-05-22T23:37:13Z

refs: #9207

Description

Working balances through makeFakeBankBridge.

Security Considerations

none, test code

Scaling Considerations

none, test code

Documentation Considerations

Contract devs may want to use these utilities eventually. I think they're too early to document yet.

Testing Considerations

per se

Upgrade Considerations

none, test code

cloudflare-workers-and-pages · 2024-05-22T23:40:19Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`ce7886e`
Status:	✅ Deploy successful!
Preview URL:	https://6aedfe14.agoric-sdk.pages.dev
Branch Preview URL:	https://9207-fake-bank.agoric-sdk.pages.dev

View logs

packages/vats/test/fake-bridge.test.js

0xpatrickdev · 2024-05-23T18:37:26Z

packages/vats/src/vat-bank.js

@@ -882,25 +881,27 @@ export function buildRootObject(_vatPowers, _args, baggage) {
        'denomToAddressUpdater',
      );

-      /** @param {ERef<import('./types.js').ScopedBridgeManager<'bank'>>} [bankBridgeMgr] */
-      async function getBankChannel(bankBridgeMgr) {
+      async function getBankChannel() {


Does this change mean vat-bank needs to be upgraded with the next release?

It's a refactor that shouldn't affect the behavior. @michaelfig WDYT, should I revert?

I haven't looked carefully, but if the only call to getBankChannel is on line 903-904, I'm okay with the refactor.

packages/vats/test/vat-bank.test.js

packages/vats/tools/fake-bridge.js

dckc · 2024-05-23T23:34:45Z

packages/vats/src/virtual-purse.js

-    retain: M.callWhen(PaymentShape).optional(amountShape).returns(amountShape),
+    retain: M.callWhen(PaymentShape)
+      .optional(AmountShapeShape)
+      .returns(amountShape),


Upgrade Consideration: That's a change to vat-bank. Are we planning to upgrade it?
If not, we might have to work around this instead.

I dunno but it's a bug in the interface guard. Working around it means never using that parameter.

If you're proposing to remove the parameter rather than fixing it (so nobody using a thing that will break in production) I can get behind that.

Since all amounts are keys, and all keys are patterns, anything that passes the old incorrect amountShape guard (when that is the same as AmountShape) will pass either the new M.pattern() or AmountShapeShape guard, so this seems like a compatible change. It only turns correct-but-rejected arguments into correctly-accepted arguments.

Yeah, upgrading this in production won't break any code that had managed to run in production before.

My remaining concern is that if we make this fix and don't ship it for a while then in the meantime people may right code that works in dev but will fail when they get to the production chain (or test in a3p). I'll take any changes out fo this PR and leave it for #9407

...people may right code that works in dev but will fail when they get to the production chain...

yes. that's exactly the upgrade consideration that I have in mind.

I'm proposing that until we have plans to deploy this fix, we don't put any code that depends on it into stuff that we plan to deploy.

If you're proposing to remove the parameter rather than fixing it ...

I hadn't considered that. And I don't advocate it now (though I suppose I wouldn't object to it).

packages/vats/src/virtual-purse.js

erights · 2024-05-24T03:02:39Z

packages/vats/src/virtual-purse.js

@@ -50,7 +51,9 @@ export const makeVirtualPurseKitIKit = (
  });

  const RetainRedeemI = M.interface('RetainRedeem', {
-    retain: M.callWhen(PaymentShape).optional(amountShape).returns(amountShape),
+    retain: M.callWhen(PaymentShape)
+      .optional(AmountShapeShape)


The original here is definitely a bug. But why not change to

Suggested change

.optional(AmountShapeShape)

.optional(M.pattern())

?
It would be more consistent with the existing ERTP guards.

If you want to abstract this by having ERTP

export AmountShapeShape = M.pattern();

in order to encapsulate that representation decision, that would be fine. But in that case, we should also fix the deposit, receive and burn guards to use the clearer name as well, for consistency.

I do like having the named shape there, and do the same for the methods for consistency.

But why define AmountShapeShape as simply M.pattern()? Isn't this more accurate and helpful?

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

Isn't this more accurate and helpful?

It is more precise, in that it admits fewer patterns as valid. But the existing guards in ERTP already admit, for example, M.any() and an optAmountShape argument, which this more precise pattern would reject. So I'd say it is less accurate and helpful because it is overly precise. Were we to make this change consistently, some (hypothetical?) programs that are currently correct would start failing.

See #9410

erights · 2024-05-24T03:08:49Z

packages/vats/src/virtual-purse.js

-    retain: M.callWhen(PaymentShape).optional(amountShape).returns(amountShape),
+    retain: M.callWhen(PaymentShape)
+      .optional(AmountShapeShape)
+      .returns(amountShape),


Since all amounts are keys, and all keys are patterns, anything that passes the old incorrect amountShape guard (when that is the same as AmountShape) will pass either the new M.pattern() or AmountShapeShape guard, so this seems like a compatible change. It only turns correct-but-rejected arguments into correctly-accepted arguments.

michaelfig

LGTM. A lot of forward progress!

closes: XXX refs: #9402 (comment) #9402 (comment) #9407 ## Description #9407 explains that sometimes we used a `.optional(AmountShape)` guard to describe an `optAmountShape` argument. #9402 (comment) #9402 (comment) discussed ways to fix this that were ultimately omitted from #9402 . This PR provides a form of the fix discussed in those comments. Three remaining controversies I'd like my reviewers to comment on: - Whether the name of the exported pattern should be `AmountShapeShape` or `AmountPatternShape`. See my doc-comment on `AmountPatternShape` for my weak reasoning about why I chose this name. - If we do go with `AmountPatternShape` for the stated reasons, should the corresponding parameter variables be changed from `optAmountShape` to `optAmountPattern`. - Whatever the name of the exported pattern, should it be defined as - `M.pattern()` - `harden({ brand: M.pattern(), value: M.pattern() })` - `harden({ brand: BrandShape, value: M.pattern() })` - something else ### Security Considerations For almost all of the choices of resolving the above controversies, this PR remains a pure bug fix. All correct code that used to work will continue working the same way, and some correct code that used to incorrectly fail due to this bug will now start working correctly. The exception would be if we both accepting the renaming of some existing occurrences of `M.pattern()` to `AmountPatternShape`/`AmountShapeShape`, and define this name as a pattern narrower than `M.pattern()`. In that case, existing (hypothetical) code that, for example, used `M.any()` in such an argument position would start failing. ### Scaling Considerations None ### Documentation Considerations None ### Testing Considerations #9407 mentions how to reproduce the bug it reports. This PR should add that as a test, to verify that this PR fixes that bug. ### Upgrade Considerations #9402 (comment) and #9402 (comment) explain the upgrade concern that was likely the cause for omitting this fix from #9042

turadg changed the base branch from master to 9207-bridge May 22, 2024 23:37

Base automatically changed from 9207-bridge to master May 23, 2024 00:06

michaelfig reviewed May 23, 2024

View reviewed changes

packages/vats/test/fake-bridge.test.js Outdated Show resolved Hide resolved

turadg mentioned this pull request May 23, 2024

feat(localchain): add transfer method to LocalChainAccountKit holder #9380

Merged

turadg force-pushed the 9207-fake-bank branch 3 times, most recently from 2cc5bbb to 14f700a Compare May 23, 2024 18:18

turadg requested a review from 0xpatrickdev May 23, 2024 18:19

turadg marked this pull request as ready for review May 23, 2024 18:20

0xpatrickdev approved these changes May 23, 2024

View reviewed changes

dckc reviewed May 23, 2024

View reviewed changes

turadg mentioned this pull request May 23, 2024

faulty amountShape param in interface guard #9407

Closed

erights reviewed May 24, 2024

View reviewed changes

turadg added 9 commits May 24, 2024 08:15

chore(types): BridgeHandler is Remote

9f57fd8

refactor(bank): re-use manager in scope

dfd77dc

feat: fake bank bridge

9abd093

test: bank balances

b71744d

fix(fake-bridge): use when to handle vows

8907806

chore(types): bank assets are 'nat'

030caf4

feat(lca): getBalance

bd0708b

feat(tools): issuerKit on AmountUtils

65dad73

test: makeFakeBankManagerKit

ce7886e

turadg force-pushed the 9207-fake-bank branch from 50f9493 to ce7886e Compare May 24, 2024 16:19

turadg requested review from 0xpatrickdev and michaelfig May 24, 2024 16:23

michaelfig approved these changes May 24, 2024

View reviewed changes

turadg added the automerge:rebase Automatically rebase updates, then merge label May 24, 2024

mergify bot merged commit 60cffcf into master May 24, 2024
71 checks passed

mergify bot deleted the 9207-fake-bank branch May 24, 2024 17:59

erights mentioned this pull request May 24, 2024

refactor(ERTP,vats): AmountPatternShape #9410

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

9207 fake bank #9402

9207 fake bank #9402

turadg commented May 22, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented May 22, 2024 •

edited

Loading

0xpatrickdev May 23, 2024

turadg May 23, 2024

michaelfig May 23, 2024

dckc May 23, 2024

turadg May 23, 2024

erights May 24, 2024

turadg May 24, 2024

dckc May 24, 2024

erights May 24, 2024

erights May 24, 2024

erights May 24, 2024

turadg May 24, 2024

erights May 24, 2024

erights May 24, 2024

michaelfig left a comment

9207 fake bank #9402

9207 fake bank #9402

Conversation

turadg commented May 22, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented May 22, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfig left a comment

Choose a reason for hiding this comment

turadg commented May 22, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented May 22, 2024 •

edited

Loading