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

katelynsills · 2021-02-05T03:59:26Z

This is the alternative to #2310 which does not require brand.mathKind

erights

Since this is draft, the only substantive comment is the incompatibility of sniffing and maintaining a semantic distinction between set and strSet. I see only three ways forward, but I may be missing other options.

Don't sniff. Use some kind of indicator instead. If so, we already know it's a separate conversation to figure out what that indicator is. Wherever we put it for now, we'll change where it is once we have auxiliary data for Una.
Drop strSet completely. Just use set to do the job of both. As a temporary measure until we get auxiliary data on Una, this is my favorite.
Drop the semantic significance of strSet, but somehow arrange to use it as an internal optimization when it applies. Given that this is all just temporary anyway until we have auxiliary data on Una, I think this internal optimization route is more trouble than it is worth.

erights · 2021-02-08T02:35:15Z

packages/ERTP/src/types.js

 * Extract and return the value.
 *
- * @property {() => Amount} getEmpty
+ * @property {(string: mathKind) => Amount} makeEmpty


Generally we've made types begin upper case. Should this be MathKind?

No. I see. We already have a program variable named MathKind, which also follows a naming convention --- how we name static namespace objects. That's unfortunate; we need a more coherent set of conventions. We should bikeshed before this gets merged, but later.

In any case, I notice you only refer to this type but do not define it.

erights · 2021-02-08T02:46:46Z

packages/ERTP/src/amountMath.js

-    helpers !== undefined,
-    details`unrecognized amountMathKind: ${amountMathKind}`,
+    Array.isArray(value),
+    details`value ${value} must be a number or an array`,


When an error is complaining for a transient reason, I generally like to phrase the message so users know to expect change here. Perhaps a "currently" or a "yet".

erights · 2021-02-08T02:53:42Z

packages/ERTP/src/amountMath.js

+  if (typeof value[0] !== 'string') {
+    return setMathHelpers;
+  }
+  return strSetMathHelpers;


All the issues with this PR hinge on this. If we sniff, then we cannot distinguish set vs strSet for empty. That's ok if strSet is only an internal optimization, that's observable only in the exposed representation. But then MATH.STRING_SET should be removed. In this PR is not only present, it is also mentioned as a publicly visible thing.

If value is [], the code above will work but for a reason that's too clever, especially if uncommented. In JavaScript, if value (whether array or not) has no '0' element (yes, a string), then value[0] is undefined and typeof value[0] is 'undefined'.

Either break empty out as a separate test or at least explain this cleverness in a comment.

If value is, for example, harden(['foo', {}]) this will misclassify it as a strSet when it can only be correctly treated as a set.

erights · 2021-02-08T03:05:43Z

packages/ERTP/src/amountMath.js

+  add: (leftAmount, rightAmount, brand = undefined) => {
+    optionalBrandCheck(leftAmount, brand);
+    optionalBrandCheck(rightAmount, brand);
+    assert.equal(leftAmount.brand, rightAmount.brand);
+    return noCoerceMake(
+      sniffHelpers(leftAmount).doAdd(leftAmount.value, rightAmount.value),
+      leftAmount.brand,
+    );
+  },


I'll just use add as an example of the issue that occurs across all the binary operators. Since you look up the helper only from the left operand, even if sniffHelpers was accurate, this would try to add harden(['foo']) and harden([{}]) together with strSetMathHelpers, which would not work.

erights · 2021-02-08T03:09:19Z

packages/ERTP/src/issuer.js

+  const add = (left, right) => amountMath.add(left, right, brand);
+  const empty = amountMath.makeEmpty(amountMathKind);
+  const coerce = allegedAmount => amountMath.coerce(allegedAmount, brand);


Nice.

Since you're factoring out these others anyway, it would be surprising not to factor out subtract as part of the pile. Also pleasant.

erights · 2021-02-08T03:11:34Z

packages/ERTP/src/issuer.js

@@ -178,7 +183,10 @@ function makeIssuerKit(
    const newTotal = newPaymentBalances.reduce(add, empty);

    // Invariant check
-    assert(amountMath.isEqual(total, newTotal), 'rights were not conserved');
+    assert(
+      amountMath.isEqual(total, newTotal, brand),


Less compelling but still pleasant to factor out isEqual as well.

Suggested change

amountMath.isEqual(total, newTotal, brand),

isEqual(total, newTotal),

erights · 2021-02-08T03:12:45Z

packages/ERTP/src/issuer.js

+        const newPurseBalance = amountMath.subtract(
+          currentBalance,
+          amount,
+          brand,
+        );


See next comment.

Suggested change

const newPurseBalance = amountMath.subtract(

currentBalance,

amount,

brand,

);

const newPurseBalance = subtract(currentBalance, amount);

erights · 2021-02-08T03:15:13Z

packages/ERTP/src/issuer.js

@@ -266,7 +275,7 @@ function makeIssuerKit(
    splitMany: (paymentP, amounts) => {
      return E.when(paymentP, srcPayment => {
        assertKnownPayment(srcPayment);
-        amounts = amounts.map(amountMath.coerce);
+        amounts = amounts.map(coerce);


Nice payoff for factoring out coerce that way.

erights · 2021-02-08T03:16:08Z

packages/ERTP/src/issuer.js

        const paymentAmountB = amountMath.subtract(
          srcPaymentBalance,
          paymentAmountA,
+          brand,
        );


Suggested change

const paymentAmountB = amountMath.subtract(

srcPaymentBalance,

paymentAmountA,

brand,

);

const paymentAmountB = subtract(srcPaymentBalance, paymentAmountA);

erights · 2021-02-08T03:18:00Z

packages/zoe/src/contracts/autoswap.js

-  const {
-    issuer: liquidityIssuer,
-    amountMath: liquidityMath,
-  } = liquidityMint.getIssuerRecord();
+  const { issuer: liquidityIssuer } = liquidityMint.getIssuerRecord();
  let liqTokenSupply = 0;

  // In order to get all the brands, we must call zcf.getTerms() after
  // we create the liquidityIssuer
-  const {
-    brands,
-    maths: { Central: centralMath, Secondary: secondaryMath },
-  } = zcf.getTerms();


Awesome to see all this melt away.

katelynsills · 2021-02-08T19:20:22Z

Drop the semantic significance of strSet, but somehow arrange to use it as an internal optimization when it applies. Given that this is all just temporary anyway until we have auxiliary data on Una, I think this internal optimization route is more trouble than it is worth.

I think this is the correct path forward. The internal optimization work has already been done for the most part, so I think we should take advantage of it, although we should probably add it back in slowly in a later PR once we have things functionally working.

For me, the sticking point that causes me to get rid of strSet as a separate mathKind is that the empty array value ([]) cannot be distinguished as strSet or set, and if we default to one or the other, later operations will have problems if we misjudged.

Additionally, I think we should restrict the elements within the value arrays to be either all strings or all records/presences. We know of no use cases that have a mixed array, and it is much easier to loosen restrictions later on if we need to, than it is to add restrictions and force users to change their habits.

erights · 2021-02-08T22:46:08Z

Additionally, I think we should restrict the elements within the value arrays to be either all strings or all records/presences. We know of no use cases that have a mixed array, and it is much easier to loosen restrictions later on if we need to, than it is to add restrictions and force users to change their habits.

This is a semantic special case non-uniformity that needs to be explained, in order to do an optimization which we can postpone. Why prohibit just strings? What about numbers? If this was well motivated, perhaps. But I don't see it. Simply dropping strSet gives us everything we need except this optimization. And it is still temporary.

Let's talk about this next meeting.

katelynsills · 2021-02-08T22:53:36Z

This is a semantic special case non-uniformity that needs to be explained, in order to do an optimization which we can postpone. Why prohibit just strings? What about numbers? If this was well motivated, perhaps. But I don't see it. Simply dropping strSet gives us everything we need except this optimization. And it is still temporary.

Since we decide the semantics, I don't think it's a non-uniformity to enforce that MathKind.SET can only contain presences and records. It's important for UI rendering that we can anticipate the structure of the values and that they are uniform. I've added this to the Zoe/ERTP meeting agenda.

katelynsills · 2021-03-03T01:58:38Z

Closed in favor of #2561

katelynsills requested a review from erights February 5, 2021 03:59

katelynsills self-assigned this Feb 5, 2021

katelynsills force-pushed the kate-amountMath-refactor-sniff branch from 9e39e55 to 2823f5f Compare February 5, 2021 19:13

erights reviewed Feb 8, 2021

View reviewed changes

katelynsills added ERTP package: ERTP Beta labels Feb 9, 2021

katelynsills added this to the Beta Launch milestone Feb 9, 2021

katelynsills mentioned this pull request Feb 9, 2021

Refactor amountMath #2311

Closed

katelynsills force-pushed the kate-amountMath-refactor-sniff branch 2 times, most recently from b6a386c to c032ee2 Compare February 25, 2021 00:02

katelynsills added 2 commits March 1, 2021 16:02

chore: more work

64d606a

chore: fix more tests

82c4b1a

katelynsills force-pushed the kate-amountMath-refactor-sniff branch from c032ee2 to 82c4b1a Compare March 2, 2021 00:02

katelynsills closed this Mar 3, 2021

katelynsills deleted the kate-amountMath-refactor-sniff branch April 12, 2021 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

katelynsills commented Feb 5, 2021

erights left a comment

erights Feb 8, 2021

erights Feb 8, 2021

erights Feb 8, 2021

erights Feb 8, 2021

erights Feb 8, 2021

erights Feb 8, 2021

erights Feb 8, 2021

erights Feb 8, 2021

erights Feb 8, 2021

erights Feb 8, 2021

erights Feb 8, 2021

erights Feb 8, 2021

katelynsills commented Feb 8, 2021

erights commented Feb 8, 2021

katelynsills commented Feb 8, 2021

katelynsills commented Mar 3, 2021

	amountMath.isEqual(total, newTotal, brand),
	isEqual(total, newTotal),

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

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

Conversation

katelynsills commented Feb 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

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 Feb 8, 2021

erights commented Feb 8, 2021

katelynsills commented Feb 8, 2021

katelynsills commented Mar 3, 2021