feat: add static amountMath. Backwards compatible with old amountMath #2561

katelynsills · 2021-03-03T01:55:55Z

This PR adds a static version of amountMath that can be imported directly from @agoric/ertp:

  import { amountMath } from '@agoric/ertp';
  const payment = mint.mintPayment(amountMath.make(837n, brand));

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

An "absolute" check on the brand in the amounts, against the brand directly from the issuer.
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);

ERTP still retains the old amountMath for backwards compatibility, but it has been deprecated. Currently, only the ERTP tests have been converted to use the new amountMath.

TODO: Remove the deprecated amountMath for testing, and convert the rest of agoric-sdk and dapps to the new amountMath.

Starts on #2311

#documentation-branch: redo-amount-math

katelynsills · 2021-03-03T02:34:08Z

Still have some linting issues - converting to draft

Chris-Hibbert

It's tough to tell whether this change will be an improvement in usability and readability. I still think it's likely, but there isn't enough usage in code in vats to tell.

packages/ERTP/src/issuer.js

packages/ERTP/src/mathHelpers/setMathHelpers.js

packages/ERTP/test/unitTests/mathHelpers/test-natMathHelpers.js

packages/ERTP/src/amountMath.js

packages/ERTP/test/unitTests/mathHelpers/test-natMathHelpers.js

Chris-Hibbert

This LGTM!

One question, and another suggestion for clarifying an error message.

packages/ERTP/src/amountMath.js

packages/ERTP/test/unitTests/mathHelpers/test-natMathHelpers.js

Chris-Hibbert · 2021-03-04T17:44:56Z

packages/zoe/src/contractSupport/bondingCurves.js

@@ -18,23 +19,26 @@ const BASIS_POINTS = 10000n; // TODO change to 10_000n once tooling copes.
 * request, and to do the actual reallocation after an offer has
 * been made.
 *
- * @param {bigint} inputValue - the value of the asset sent
+ * @param {Value} inputValue - the value of the asset sent


Why does Typescript want this change? Is it because we can't tell it that the Amounts we started with are from NatMath types?

Yeah, Values in Amounts are either NatValues (bigints) or SetValues (arrays), so when we as programmers know that we are using a bigint, the type system doesn't know. We have two choices: do an assert.typeof(value, bigint) before we call the method that requires bigints, or we can loosen the type of the method to be a Value. That's what I chose for this method, otherwise I would have to do 3 or so asserts. We may be able to improve the types, but this is the best I know how to do. The plus side is that instead of Values being any and allowing anything in amounts, we will catch many more problems in linting.

katelynsills · 2021-03-05T02:39:04Z

This is now ready for review. @erights could you take a look? Thanks!

erights

My confusion about makeExternalStore didn't stop me from doing a differential review. But makeExternalStore does not look like anything I expected.

LGTM!

erights · 2021-03-08T05:35:45Z

packages/ERTP/src/mathHelpers/setMathHelpers.js

    assert(passStyleOf(list) === 'copyArray', 'list must be an array');
    checkForDupes(makeBuckets(list));
    return list;
  },
-  doGetEmpty: _ => identity,
+  doMakeEmpty: _ => identity,


Please remind me of the rationale for this name change?

Hmm, I don't remember it exactly, but it was the general consensus of an ERTP meeting. I think @dtribble had an opinion on it. I have no preference between them.

erights · 2021-03-08T05:39:30Z

packages/ERTP/src/types.js

+ * This version of amountMath is deprecated. Please use `amountMath` directly
+ * as exported by `@agoric/ertp`.
+ *
+ * @property {() => Brand} getBrand Deprecated


Do any tools understand this way of saying deprecated? If not but there's no better way, then fine.

No, unfortunately, there's no better way to do this. The @deprecated tag works for standalone functions only, and not on methods. So this is for humans reading the types. That said, makeAmountMath and makeLocalAmountMath are functions and they do have the @deprecated tag so the tools should probably understand that the usual pattern of usage is deprecated.

erights · 2021-03-08T05:40:29Z

packages/ERTP/test/swingsetTests/splitPayments/test-splitPayments.js

@@ -1,4 +1,9 @@
+// @ts-check
+
+// eslint-disable-next-line import/no-extraneous-dependencies


Have we ever figured out why we need these?

No, sorry, it's very mysterious to me why I get errors for this in VSCode. The settings in packages/eslint-config should silence this, and they do for the linting at the command line, but not for the "red squigglies". All of the my VSCode settings appear to be correct, and I use the ESLint extension, so I'm at a loss.

I guess a good question is, does anyone else see these as red squigglies? If it's just me, I should probably take out the eslint-disable-next-line.

I see them often.

erights · 2021-03-08T05:41:39Z

packages/ERTP/test/swingsetTests/splitPayments/vat-alice.js

+    makeAliceMaker() {
+      return makeAliceMaker(vatPowers.testLog);


Is the removal of host related to the rest of this PR?

It's unrelated.

erights · 2021-03-08T05:56:57Z

packages/ERTP/src/amountMath.js

  mustBeComparable(brand);
-  assert.typeof(amountMathKind, 'string');
-
-  const mathHelpers = {
-    nat: natMathHelpers,
-    strSet: strSetMathHelpers,
-    set: setMathHelpers,
-  };
-  const helpers = mathHelpers[amountMathKind];
+  assert(typeof brand === 'object', msg);


Perhaps replace both of these with

assert(passStyleOf(brand) === REMOTE_STYLE), msg);

In any case, when you do want to assert a typeof, assert supports that directly

assert.typeof(brand, 'object', msg);

By "both of these" I mean both the mustBeComparable and the typeof check. Then you can also drop the passStyleOf check below.

erights · 2021-03-08T06:00:08Z

packages/ERTP/src/amountMath.js

  assert(
-    helpers !== undefined,
-    X`unrecognized amountMathKind: ${amountMathKind}`,
+    passStyleOf(brand) === REMOTE_STYLE && ownKeys.every(inBrandMethods),


Interesting. I would not have thought to check that.

passStyleOf(brand) === REMOTE_STYLE &&

Having fixed the code on line 128, this part of the check on line 132 is redundant and should be removed.

op, good eye. I will remove

erights · 2021-03-08T06:13:10Z

packages/ERTP/src/amountMath.js

+    return amountMath.make(allegedAmount.value, brand);
+  },
+  getValue: (amount, brand) => amountMath.coerce(amount, brand).value,
+  makeEmpty: (mathKind, brand) => {


Here, we actually do make a new one each time, which makes sense.

packages/ERTP/src/issuer.js

katelynsills added ERTP package: ERTP Beta labels Mar 3, 2021

katelynsills added this to the Beta Initial Launch milestone Mar 3, 2021

katelynsills requested review from erights and Chris-Hibbert March 3, 2021 01:55

katelynsills self-assigned this Mar 3, 2021

This was referenced Mar 3, 2021

DRAFT FOR DESIGN ONLY: refactor amountMath w/ mathKind property on the brand presence #2310

Closed

DRAFT FOR DESIGN: Try to sniff out the mathHelpers from the amount value #2339

Closed

katelynsills added a commit to Agoric/documentation that referenced this pull request Mar 3, 2021

chore: fix the error message so it matches Agoric/agoric-sdk#2561

5371b4c

katelynsills marked this pull request as draft March 3, 2021 02:33

Chris-Hibbert reviewed Mar 3, 2021

View reviewed changes

katelynsills force-pushed the 2311-redo-amount-math-backwards-compat branch from 2dd9fb7 to 24bbd9a Compare March 4, 2021 01:25

Chris-Hibbert approved these changes Mar 4, 2021

View reviewed changes

katelynsills marked this pull request as ready for review March 5, 2021 02:38

erights approved these changes Mar 8, 2021

View reviewed changes

katelynsills added 9 commits March 8, 2021 11:59

chore: add new static amountMath. Deprecate old amountMath.

a7457d3

chore: lint-fix

3a18a50

chore: type fixes

e54da15

chore: fix some more types

6e32a2e

chore: change error messages

5c53677

chore: lint-fix

4b71253

chore: fix test

76c2dd6

chore: more lint fixes

2cb3173

chore: address PR comments

58af196

katelynsills force-pushed the 2311-redo-amount-math-backwards-compat branch from 8619635 to 58af196 Compare March 8, 2021 19:59

katelynsills enabled auto-merge (squash) March 8, 2021 20:00

katelynsills merged commit 1620307 into master Mar 8, 2021

katelynsills deleted the 2311-redo-amount-math-backwards-compat branch March 8, 2021 20:12

katelynsills mentioned this pull request Mar 8, 2021

chore: fix the error message so it matches agoric-sdk Agoric/documentation#451

Merged

katelynsills added a commit to Agoric/documentation that referenced this pull request Mar 8, 2021

chore: fix the error message so it matches Agoric/agoric-sdk#2561

2800428

katelynsills added a commit to Agoric/documentation that referenced this pull request Mar 8, 2021

chore: fix the error message so it matches Agoric/agoric-sdk#2561 (#451)

9014a5a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add static amountMath. Backwards compatible with old amountMath #2561

feat: add static amountMath. Backwards compatible with old amountMath #2561

katelynsills commented Mar 3, 2021 •

edited

Loading

katelynsills commented Mar 3, 2021

Chris-Hibbert left a comment

Chris-Hibbert left a comment

Chris-Hibbert Mar 4, 2021

katelynsills Mar 4, 2021

katelynsills commented Mar 5, 2021

erights left a comment

erights Mar 8, 2021

katelynsills Mar 8, 2021

erights Mar 8, 2021 •

edited

Loading

katelynsills Mar 8, 2021

erights Mar 8, 2021

katelynsills Mar 8, 2021

katelynsills Mar 8, 2021

erights Mar 8, 2021

erights Mar 8, 2021

katelynsills Mar 8, 2021

erights Mar 8, 2021

erights Mar 8, 2021

erights Mar 8, 2021

erights Mar 8, 2021

katelynsills Mar 8, 2021

katelynsills Mar 8, 2021

erights Mar 8, 2021

		@@ -1,4 +1,9 @@
		// @ts-check

		// eslint-disable-next-line import/no-extraneous-dependencies

feat: add static amountMath. Backwards compatible with old amountMath #2561

feat: add static amountMath. Backwards compatible with old amountMath #2561

Conversation

katelynsills commented Mar 3, 2021 • edited Loading

katelynsills commented Mar 3, 2021

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katelynsills commented Mar 5, 2021

erights left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erights Mar 8, 2021 • edited Loading

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

Choose a reason for hiding this comment

katelynsills commented Mar 3, 2021 •

edited

Loading

erights Mar 8, 2021 •

edited

Loading