-
Notifications
You must be signed in to change notification settings - Fork 206
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
Remove Pointless Excessive Abstraction #2425
Conversation
Note that this PR's CI run shows that we still have old parser breakage in our toolchain. No longer on the terminal |
See #2427 |
I disagree. |
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.
Looks fine to me. Btw, this code will be majorly rewritten in the conversion to Nat 4.0.0.
I'm going to mostly agree with your point, but disagree about the implementation of it. Nearly all zeroes and ones are indistinguishable, not because they aren't used for different things, but because the programmer can't conceive of changing them to a different number. That's not at all the case with 10_000n. I spent a couple of weeks at Niantic disentangling all the 8s in our code by purpose. (It was a game, that started out with 8 levels of players, and 8 levels for three different kinds of items, and 8 levels for some constructed objects. When we wanted 16 player levels, we needed every 8 to say what it was for.) Some 10_000s are BASIS_POINTS, which if they're used for entering parameters probably wouldn't change when someone decides we need to track holdings of stable coins to 6 decimal places. But you're right that the name of that constant shouldn't be BIG_10000. OTOH, when I added BIG_10000, at least one of the manifest bigints I replaced had the wrong number of zeroes. In the same way I replace 365 with a global YEAR, 10_000 should be replaced with a named global that says what it's for. |
@@ -41,10 +38,10 @@ export const getInputPrice = ( | |||
assert(inputReserve > 0, X`inputReserve ${inputReserve} must be positive`); | |||
assert(outputReserve > 0, X`outputReserve ${outputReserve} must be positive`); | |||
|
|||
const oneMinusFeeScaled = BIG_10000 - BigInt(feeBasisPoints); | |||
const oneMinusFeeScaled = 10_000n - BigInt(feeBasisPoints); |
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.
Please rename to BASIS_POINTS rather than using a manifest constant.
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.
yes, actual abstraction that says more than the number! Done.
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.
thanks!
17309f9
to
b1bf8ca
Compare
Just to be clear, the issue with "magic numbers" (any number other than 0,1, and maybe -1) is not that we should uselessly replace numbers with strings, but that we should replace numbers with semantic meaning. 10000n should not be replaced with |
I see no way in which
BIG_ONE
is clearer than1n
. There is one way in which the constant10000n
is unclear. You might, at a glance, mistake the number of zeros you're looking at. However,BIG_10000n
is no clearer. TC39 already addressed the readability issue of big numerical literals like that by letting us use underbars where on paper we'd use commas.10_000n
is clearer than both. It is actually clear.The issue in this PR is really a microcosm of issues raised by #2420 . For all the wonder of abstraction it can be used excessively, at a cost to readability. We need to be sensitive to both sides of that tradeoff.
Historically, the origin of some of this stuff was when we did not yet have good enough toolchain support for BigInts. Today, some of it may come from worry about the efficiency of small BigInts compared to floats (aka
numbers
). However, we know that small BigInts are as efficient as floats on all JS engines other than XS. We do not yet know whether the extra cost on XS is significant for us. But we do know that, if it is, we can fix XS rather than make our code less readable.I expect these points to be controversial. Let's have that discussion first on this PR where the issue is narrow and focused.