chore(portfolio-contract): Improve "toUSDN" comments #12080
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
2d620ea
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e2107ca1.agoric-sdk.pages.dev |
| Branch Preview URL: | https://gibson-2025-10-ymax-subtract.agoric-sdk.pages.dev |
| * reduces it by a full unit). | ||
| */ | ||
| export const subtractBasisPoints = (n: bigint, bps: bigint) => | ||
| (n * (10000n - bps)) / 10000n; |
There was a problem hiding this comment.
consider something like:
const PERCENT = 100n;
const BASIS_POINT = PERCENT * PERCENT;I think that might already be in ERTP somewhere.
There was a problem hiding this comment.
also, "subtract" seems like an odd name. We're multiplying by a ratio, right?
Is there a particular reason we're not using ERTP amounts and ratios?
There was a problem hiding this comment.
I didn't find suitable code in ERTP, but if you know of some then a pointer would be valuable regardless of this PR's fate.
There was a problem hiding this comment.
... a pointer would be valuable ...
ERTP/src/ratio.js has, for example, floorMultiplyBy.
It also has const PERCENT = 100n; though it's not exported.
There was a problem hiding this comment.
As to BASIS_POINT, we seem to use a constant but not bother importing it from a common source:
$ ack 'const BASIS_POINTS = 10000n;'
packages/inter-protocol/test/interest.test.js
17:const BASIS_POINTS = 10000n;
packages/inter-protocol/test/provisionPool.test.js
45:const BASIS_POINTS = 10000n;
packages/inter-protocol/test/vaultFactory/vaultFactoryUtils.js
25:export const BASIS_POINTS = 10000n;
packages/inter-protocol/test/vaultFactory/vault-contract-wrapper.js
28:const BASIS_POINTS = 10000n;
packages/inter-protocol/test/vaultFactory/driver.js
43:export const BASIS_POINTS = 10000n;
packages/inter-protocol/test/psm/psm.test.js
46:const BASIS_POINTS = 10000n;
packages/inter-protocol/test/interest-labeled.test.js
19:const BASIS_POINTS = 10000n;
packages/inter-protocol/src/proposals/startPSM.js
31:const BASIS_POINTS = 10000n;
packages/zoe/test/unitTests/contracts/loan/borrow.test.js
30:const BASIS_POINTS = 10000n;
packages/zoe/src/contractSupport/bondingCurves.js
9:const BASIS_POINTS = 10000n; // TODO change to 10_000n once tooling copes.
packages/zoe/src/contracts/callSpread/percent.js
5:const BASIS_POINTS = 10000n;
packages/zoe/src/contracts/callSpread/pricedCallSpread.js
23:const BASIS_POINTS = 10000n;and
$ ack 'const BASIS_POINTS = 10_000n;'
packages/inter-protocol/src/auction/auctioneer.js
45:const BASIS_POINTS = 10_000n;There was a problem hiding this comment.
ERTP/src/ratio.js has, for example,
floorMultiplyBy.
Thanks. floorMultiplyBy is definitely correct, but the domain being Amounts rather than Nats makes it far too cumbersome here. We could consider extending packages/ERTP/src/safeMath.js, but honestly applyHaircut is so straightforward that it seems fine as an isolated one-off, and would continue to exist here even if there were a nat-based floorMultiplyBy (because applyHaircut(flow, feeInBasisPoints) is much more clear than floorMultiplyBy(flow, makeRatio(BASIS_POINTS - feeInBasisPoints, BASIS_POINTS))).
There was a problem hiding this comment.
applyHaircutis so straightforward that it seems fine as an isolated one-off
ok...
I'm just uneasy about reviewing / maintaining more of this code.
| // https://github.com/Agoric/agoric-private/issues/415 | ||
| const usdnOut = | ||
| (BigInt(flow) * (10000n - BigInt(edge.variableFee))) / 10000n - 1n; | ||
| subtractBasisPoints(BigInt(flow), BigInt(edge.variableFee)) - 1n; |
There was a problem hiding this comment.
nit: since the change is invisible to tests, it's a refactor, right?
(not that the difference between "chore" and "refactor" matters much)
gibson042
force-pushed
the
gibson-2025-10-ymax-subtract-bps
branch
from
ad684d4 to
2d620ea
Compare
Ref Agoric/agoric-private#433
Description
applyHaircuthelper function