-
Notifications
You must be signed in to change notification settings - Fork 254
feat(ertp): ertp-only part of minimal want-patterns #11235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
2e4d932 to
979da2a
Compare
| */ | ||
| const multiplyHelper = (amount, ratio, divideOp) => { | ||
| AmountMath.coerce(amount.brand, amount); | ||
| amount = AmountMath.coerce(amount.brand, amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A drive-by. coerce only produces a valid output from valid or almost-valid input. It does not validate or modify the input. So either we should use the result of coerce or we should remove it. Not sure yet whether this coerce is actually needed.
| @@ -0,0 +1,60 @@ | |||
| import { test } from '@agoric/swingset-vat/tools/prepare-test-env-ava.js'; | |||
| import { M, makeCopySet } from '@endo/patterns'; | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO do we also need a copyBagBoundMathHelpers.test.js?
979da2a to
b01e796
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements the ERTP portion of minimal want patterns, introducing support for M.containerHas patterns in AmountMath.isGTE and AmountMath.subtract operations. This feature allows pattern-based matching on non-fungible and semi-fungible assets.
Key changes include:
- Added
AmountBoundtypes that extendAmountto supportHasBoundpatterns - Implemented pattern matching logic in
AmountMathforM.containerHasoperations - Updated error messages to be more descriptive and consistent
- Added comprehensive test coverage for the new pattern matching functionality
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ERTP/src/amountMath.js | Core implementation of pattern matching with new amountSplit function and updated isGTE/subtract methods |
| packages/ERTP/src/types.ts | Added HasBound, AmountBound, and related type definitions for pattern matching |
| packages/ERTP/src/typeGuards.js | Added type guards and shapes for the new bound types |
| packages/ERTP/src/purse.js | Updated purse withdraw method to accept AmountBound with placeholder for pattern support |
| packages/ERTP/test/unitTests/mathHelpers/setBoundMathHelpers.test.js | New comprehensive tests for set-based pattern matching |
| packages/ERTP/test/unitTests/mathHelpers/copySetBoundMathHelpers.test.js | New comprehensive tests for copySet-based pattern matching |
| packages/store/test/borrow-guards.js | Updated shapes to use new AmountBound types |
| Multiple test files | Updated error message assertions to match new "AmountValue" terminology |
| coerceToElements, | ||
| elementsCompare, | ||
| } from '@agoric/store'; | ||
| } from '@endo/patterns'; |
Copilot
AI
Sep 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import source has changed from '@agoric/store' to '@endo/patterns', but some functions like elementsIsSuperset, elementsDisjointUnion, and elementsDisjointSubtract may not be available in '@endo/patterns'. Verify that all imported functions are actually exported from the new module.
| setDisjointUnion, | ||
| setDisjointSubtract, | ||
| } from '@agoric/store'; | ||
| } from '@endo/patterns'; |
Copilot
AI
Sep 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import source has changed from '@agoric/store' to '@endo/patterns', but some functions like setIsSuperset, setDisjointUnion, and setDisjointSubtract may not be available in '@endo/patterns'. Verify that all imported functions are actually exported from the new module.
| ), | ||
| { brand: mockBrand, value: makeCopySet(['b']) }, | ||
| `['a', 'b'] - ['a'] = ['a']`, | ||
| `['a', 'b'] - ['a'] = ['b']`, |
Copilot
AI
Sep 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment describes the subtraction operation incorrectly. It should say ['a', 'b'] - ['a'] = ['b'] which matches the expected result in the test assertion above.
| `['a', 'b'] - ['a'] = ['b']`, | |
| `['a', 'b'] - ['a'] = ['b']`, // ['a', 'b'] minus ['a'] equals ['b'] |
closes: #XXXX
refs: endojs/endo#2710 #1905 #1913 #1915 #2155 #2156 #2230 #4212 #4217 #10951 #10952
Description
Implements the ERTP part of minimal want patterns, where the only supported pattern is
M.containerHas. #10952 then implements the zoe portion.Security Considerations
none
Scaling Considerations
at the ERTP level, none.
Documentation Considerations
Need to update ERTP docs to explain the use of
M.haspatterns inAmountMath.isGTEandAmountMath.subtract.Testing Considerations
ERTP tests already added.
Upgrade Considerations
None