refactor(ERTP,vats): AmountPatternShape #9410
Conversation
Deploying agoric-sdk with
|
| 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 |
There was a problem hiding this comment.
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.
|
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. |
|
@mhofman @turadg see #9410 (comment) for an explanation of a recent change. PTAL. Attn @JD-Lorax |
There was a problem hiding this comment.
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 ?
|
@turadg , you requested changes at #9410 (review) writing:
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
changed the title
Good point. Done. PTAL. |
erights
force-pushed
the
markm-amount-shape-shape
branch
from
e22fd40
to
7f538c6
Compare
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
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.
closes: XXX
refs: #9402 (comment) #9402 (comment) #9407
Description
#9407 explains that sometimes we used a
.optional(AmountShape)guard to describe anoptAmountShapeargument.#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:
AmountShapeShapeorAmountPatternShape. See my doc-comment onAmountPatternShapefor my weak reasoning about why I chose this name.AmountPatternShapefor the stated reasons, should the corresponding parameter variables be changed fromoptAmountShapetooptAmountPattern.M.pattern()harden({ brand: M.pattern(), value: M.pattern() })harden({ brand: BrandShape, value: M.pattern() })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()toAmountPatternShape/AmountShapeShape, and define this name as a pattern narrower thanM.pattern(). In that case, existing (hypothetical) code that, for example, usedM.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