refactor(ERTP,vats): AmountPatternShape

[erights]

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

[dckc]

This fixes the bug, but until we have a plan to upgrade vat-bank, clients have to not depend on any fix. I don't know how to manage that if we merge to master at this time, so I defer to others to approve.

[cloudflare-workers-and-pages]

Deploying agoric-sdk with    Cloudflare Pages

Latest commit:	7f538c6
Status:	✅  Deploy successful!
Preview URL:	https://a849a8ff.agoric-sdk.pages.dev
Branch Preview URL:	https://markm-amount-shape-shape.agoric-sdk.pages.dev

View logs

[turadg]

How does this fix #9407? I see only a refactor creating AmountPatternShape as an alias for M.pattern().

Please include a test showing the behavior that failed before this PR and passes after.

[mhofman]

Upgrade Considerations

#9402 (comment) and #9402 (comment) explain the upgrade concern that was likely the cause for omitting this fix from #9042

The comment by @turadg states:

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'm not sure in what vat the virtual-purse.js which contains this guard change ends up in, but as long as it is upgraded before any of its consumers that assume the new behavior, we should be ok. Same story for localchain. Landing this change in master affectively requires us to upgrade these vats in the next release. As such, the next upgrade handler should have code to upgrade these vats explicitly (since we do not yet have generic code to upgrade all vats). This PR should introduce that change. Also since this change is somewhat subtle, I'm wondering if we need a code comment for future reviewers of release changes stating the nature of this specific change.

[erights]

Originally this PR 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 this PR 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.

[erights]

@mhofman @turadg see #9410 (comment) for an explanation of a recent change. PTAL.

Attn @JD-Lorax

[mhofman]

Reviewed purely on the upgradability aspect, and to match the approach we discussed. I didn't review if the choice of pattern itself is appropriate, nor the test itself.

Regarding the PR itself, from what I understanding this is now effectively a refactor and no longer a fix, right ?

[erights]

@turadg , you requested changes at #9410 (review) writing:

How does this fix #9407? I see only a refactor creating AmountPatternShape as an alias for M.pattern().

Please include a test showing the behavior that failed before this PR and passes after.

That part is long done. If that remains your only concern and you're satisfied with the answer, could you please clear your requested changes so that I remain blocked only on @mhofman 's concerns? Thanks.

[erights]

Regarding the PR itself, from what I understanding this is now effectively a refactor and no longer a fix, right ?

Good point. Done. PTAL.