Skip to content
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

Refactor amountMath #2311

Closed
katelynsills opened this issue Feb 2, 2021 · 2 comments
Closed

Refactor amountMath #2311

katelynsills opened this issue Feb 2, 2021 · 2 comments
Assignees
Labels
enhancement New feature or request Epic

Comments

@katelynsills
Copy link
Contributor

katelynsills commented Feb 2, 2021

What is the Problem Being Solved?

Manipulating amounts using amountMath is awkward in a few ways. First, amountMath is an object which has methods, which makes it a Remotable and not pass-by-copy. This means that a user cannot simply get the amountMath from a remote issuer, as a remote amountMath would require a round trip for manipulating amounts. Instead, the user must make a local amountMath object.

However, in user code, this requires making the local amountMath for each brand and then passing them around to every function which wants to manipulate amounts, which is a chore.

Description of the Design

amountMath code should dispatch to the correct mathHelpers. This means that we no longer need to have an amountMath maker function that takes a mathKind as a parameter and makes an object.

Refactoring AmountMath

This PR shows the proposed refactoring of AmountMath: #2339

The goal is to be able to import amountMath from @agoric/ertp, and be able to use it to add, subtract, etc.

AmountMath was serving two purposes simultaneously, which can be teased out separately:

  1. An "absolute" check on the brand in the amounts, against the brand directly from the issuer.
  2. A "relative" check to ensure that we are manipulating amounts of the same brand, regardless of whether the brands in the amounts match the brand directly from the issuer.

This refactoring tries to separate these two purposes. For instance, in add, if an absolute check is necessary, you must pass the brand that you got from the issuer (or from Zoe) as an optional third parameter:

amountMath.add(amount1, amount2, brandFromIssuer)

If only a relative check is required, you can leave out the optional brand, and add will error if the two amounts have different brands:

amountMath.add(amount1, amount2);

If this refactoring or something like it is sufficient, that means that we can do without having synchronous properties on brands.

Getting rid of StringSet MathHelpers

In #2339, we learned that because [] as a value could either belong to MathKind.SET or MathKind.STRING_SET, we cannot reliably distinguish between MathKind.SET and MathKind.STRING_SET. Moreover, MathKind.SET can include strings. Therefore, we are getting rid of MathKind.STRING_SET and replacing it with MathKind.SET, meaning that there would only be NAT and SET.

In the Amount Patterns Implementation plan, we realized that we may need a deterministic ordering. This was possible with STRING_SET, but is not possible with SET, as presences cannot be ordered. Given that we were going to have this problem with SET always, I think it is ok that we punt the solution until after this refactoring is done.

Security Considerations

  • If only a relative check is done, the absolute check that was embedded in all the amountMath methods is not done. We should be careful in our migration to include the absolute check unless unnecessary.

Test Plan

  1. Refactor AmountMath as in DRAFT FOR DESIGN: Try to sniff out the mathHelpers from the amount value #2339
  2. Ensure the current tests pass, including tests of the amountMath specifically.
@katelynsills
Copy link
Contributor Author

  • Rename M to amountMath
  • Explore the possibility of sniffing the mathKind based on the structure of the value.

@katelynsills katelynsills added the ERTP package: ERTP label Feb 9, 2021
@katelynsills katelynsills self-assigned this Feb 9, 2021
@katelynsills katelynsills added this to the Beta Launch milestone Feb 9, 2021
@katelynsills katelynsills changed the title Refactor amountMath, given mathKind property on the brand presence Refactor amountMath Feb 9, 2021
@katelynsills katelynsills removed the ERTP package: ERTP label Mar 9, 2021
@rowgraus rowgraus removed this from the Beta Initial Launch milestone Mar 23, 2021
@katelynsills
Copy link
Contributor Author

This is closed as of #2894

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Epic
Projects
None yet
2 participants