Skip to content

chore(portfolio-contract): Improve "toUSDN" comments #12080

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

chore(portfolio-contract): Improve "toUSDN" comments #12080

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
11dfea7
chore(portfolio-contract): Improve "toUSDN" comments
gibson042 Oct 8, 2025
2d620ea
refactor(portfolio-contract): Define a `applyHaircut` helper function
gibson042 Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions packages/portfolio-contract/tools/plan-solve.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ const replaceOrInit = <K, V>(
map.set(key, callback(old, key, exists));
};

/**
* Reduce a value by a natural number of basis points, rounding the result down
* (e.g., 1 bp of 10k is exactly 1, so subtracting 1 bp from any value up to 10k
* reduces it by a full unit).
*/
export const applyHaircut = (n: bigint, bps: bigint) =>
(n * (10000n - bps)) / 10000n;
Copy link
Member

@dckc dckc Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider something like:

const PERCENT = 100n;
const BASIS_POINT = PERCENT * PERCENT;

I think that might already be in ERTP somewhere.

Copy link
Member

@dckc dckc Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@gibson042 gibson042 Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@dckc dckc Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... a pointer would be valuable ...

ERTP/src/ratio.js has, for example, floorMultiplyBy.

It also has const PERCENT = 100n; though it's not exported.

Copy link
Member

@dckc dckc Oct 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Member Author

@gibson042 gibson042 Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))).

Copy link
Member

@dckc dckc Oct 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

applyHaircut is so straightforward that it seems fine as an isolated one-off

ok...

I'm just uneasy about reviewing / maintaining more of this code.


// eslint-disable-next-line @typescript-eslint/no-unused-vars
const trace = makeTracer('solve');

Expand Down Expand Up @@ -446,11 +454,14 @@ export const rebalanceMinCostFlowSteps = async (
break;
}
case 'toUSDN': {
// NOTE USDN transfer incurs a fee on output amount in basis points
// HACK of subtract 1n in order to avoid rounding errors in Noble
// See https://github.com/Agoric/agoric-private/issues/415
// USDN transfer imposes a "haircut" fee on the *amount*.
// Expose the post-fee net output as `detail.usdnOut`.
// TODO: Apply this in both directions per
// https://github.com/Agoric/agoric-private/issues/433
// HACK: subtract 1n to avoid rounding errors in Noble per
// https://github.com/Agoric/agoric-private/issues/415
const usdnOut =
(BigInt(flow) * (10000n - BigInt(edge.variableFee))) / 10000n - 1n;
applyHaircut(BigInt(flow), BigInt(edge.variableFee)) - 1n;
details = { detail: { usdnOut } };
break;
}
Expand Down
Loading