Skip to content

Commit

Permalink
Ratio.quantize() shouldn't increase precision unnecessarily (#9926)
Browse files Browse the repository at this point in the history
## Description

Ratios are designed to hold fractions as ratios with arbitrary length denominators. A few operations (multiplication) can cause the denominators to grow rapidly. To allow holders of growing ratios to manage this, we provide a method `quantize()`, which returns a ratio that approximates the original value, but with a smaller denominator.

The current implementation uses the provided denominator even if it is larger. This doesn't gain anything, and can lose precision unnecessarily

### Security Considerations

No security implications.

### Scaling Considerations

The quantize() method is used to limit the size of very large BigInts. This improvement only makes that better.

### Documentation Considerations

No need for changes. The current doc does say "Make an equivalant ratio with a new denominator", even though the result is only an approximation.

### Testing Considerations

Added new tests.

### Upgrade Considerations

No hurry to get this included in any particular contract upgrade.
  • Loading branch information
mergify[bot] authored Aug 20, 2024
2 parents 8981d86 + 118e6ad commit 9986152
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 25 deletions.
18 changes: 10 additions & 8 deletions packages/inter-protocol/test/auction/sortedOffers.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ import {
ratiosSame,
makeRatioFromAmounts,
quantize,
subtractRatios,
} from '@agoric/zoe/src/contractSupport/index.js';
import { setup } from '@agoric/zoe/test/unitTests/setupBasicMints.js';
import { AmountMath } from '@agoric/ertp';
import {
fromPriceOfferKey,
toPriceOfferKey,
Expand Down Expand Up @@ -65,6 +67,10 @@ test('toKey discount', t => {
t.true(keyD26 > keyD25);
});

const ratiosEqual = (t, left, right) => {
t.true(AmountMath.isEmpty(subtractRatios(left, right).numerator));
};

test('fromKey Price', t => {
const { moola, moolaKit, simoleans, simoleanKit } = setup();
const { brand: moolaBrand } = moolaKit;
Expand All @@ -81,12 +87,7 @@ test('fromKey Price', t => {
t.true(
ratiosSame(priceAOut, makeRatioFromAmounts(moola(40n * N), simoleans(N))),
);
t.true(
ratiosSame(
priceBOut,
quantize(makeRatioFromAmounts(moola(40n), simoleans(1000n)), N),
),
);
ratiosEqual(t, priceBOut, makeRatioFromAmounts(moola(40n), simoleans(1000n)));
t.is(timeA, DEC25);
t.is(timeB, DEC25);
});
Expand All @@ -104,8 +105,9 @@ test('fromKey discount', t => {

const [discountAOut, timeA] = fromScaledRateOfferKey(keyA25, moolaBrand, 9);
const [discountBOut, timeB] = fromScaledRateOfferKey(keyB25, moolaBrand, 9);
t.deepEqual(quantize(discountAOut, 10000n), quantize(fivePercent, 10000n));
t.deepEqual(
ratiosEqual(t, discountAOut, fivePercent);
ratiosEqual(
t,
quantize(discountBOut, 10000n),
quantize(fivePointFivePercent, 10000n),
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,11 @@ Generated by [AVA](https://avajs.dev).
compoundedInterest: {
denominator: {
brand: Object @Alleged: IST brand {},
value: 100000000000000000000n,
value: 857701650301172500n,
},
numerator: {
brand: Object @Alleged: IST brand {},
value: 101967213114754098360n,
value: 874574469651359500n,
},
},
interestRate: {
Expand Down Expand Up @@ -387,11 +387,11 @@ Generated by [AVA](https://avajs.dev).
interest: {
denominator: {
brand: Object @Alleged: IST brand {},
value: 100000000000000000000n,
value: 857701650301172500n,
},
numerator: {
brand: Object @Alleged: IST brand {},
value: 101967213114754098360n,
value: 874574469651359500n,
},
},
},
Expand All @@ -413,11 +413,11 @@ Generated by [AVA](https://avajs.dev).
interest: {
denominator: {
brand: Object @Alleged: IST brand {},
value: 100000000000000000000n,
value: 857701650301172500n,
},
numerator: {
brand: Object @Alleged: IST brand {},
value: 101967213114754098360n,
value: 874574469651359500n,
},
},
},
Expand Down
Binary file not shown.
7 changes: 6 additions & 1 deletion packages/zoe/src/contractSupport/ratio.js
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,8 @@ export const ratiosSame = (left, right) => {
};

/**
* Make an equivalant ratio with a new denominator
* Make a new ratio with a smaller denominator that approximates the ratio. If
* the proposed denominator is larger than the current one, return the original.
*
* @param {Ratio} ratio
* @param {bigint} newDen
Expand All @@ -352,6 +353,10 @@ export const ratiosSame = (left, right) => {
export const quantize = (ratio, newDen) => {
const oldDen = ratio.denominator.value;
const oldNum = ratio.numerator.value;
if (newDen > oldDen) {
return ratio;
}

const newNum =
newDen === oldDen ? oldNum : bankersDivide(oldNum * newDen, oldDen);
return makeRatio(
Expand Down
41 changes: 31 additions & 10 deletions packages/zoe/test/unitTests/contractSupport/ratio.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -501,25 +501,25 @@ const { brand } = makeIssuerKit('moe');

test('ratio - quantize', t => {
/** @type {Array<[numBefore: bigint, denBefore: bigint, numAfter: bigint, denAfter: bigint]>} */
const cases = /** @type {const} */ [
const cases = [
[1n, 1n, 1n, 1n],
[10n, 10n, 10n, 10n],
[2n * 10n ** 9n, 1n * 10n ** 9n, 20n, 10n],

[12345n, 12345n, 100n, 100n],
[12345n, 12345n, 100000n, 100000n],
[12345n, 12345n, 10n ** 15n, 10n ** 15n],

[12345n, 123n, 100365854n, 10n ** 6n],
[12345n, 123n, 10036585n, 10n ** 5n],
[12345n, 123n, 1003659n, 10n ** 4n],
[12345n, 123n, 100366n, 10n ** 3n],
[12345n, 123n, 10037n, 10n ** 2n],
[12345n, 123n, 1004n, 10n ** 1n],
[12345n, 123n, 100n, 10n ** 0n],

[12345n, 12345n, 100n, 100n],
];

for (const [numBefore, denBefore, numAfter, denAfter] of cases) {
for (const [
numBefore,
denBefore,
numAfter,
target,
denAfter = target,
] of cases) {
const before = makeRatio(numBefore, brand, denBefore, brand);
const after = makeRatio(numAfter, brand, denAfter, brand);
t.deepEqual(
Expand All @@ -530,6 +530,27 @@ test('ratio - quantize', t => {
}
});

test('ratio - quantize - leave it alone', t => {
const cases = [
[12345n, 123n, 10n ** 5n, 12345n, 123n],
[12345n, 123n, 10n ** 4n, 12345n, 123n],
[12345n, 123n, 10n ** 3n, 12345n, 123n],

[12345n, 12345n, 100_000n, 12345n, 12345n],
[12345n, 12345n, 10n ** 15n, 12345n, 12345n],
];

for (const [numPre, denPre, qTarget, numAfter, denAfter] of cases) {
const before = makeRatio(numPre, brand, denPre, brand);
const after = makeRatio(numAfter, brand, denAfter, brand);
t.deepEqual(
quantize(before, qTarget),
after,
`${numPre}/${denPre} quantized to ${qTarget} should be ${numAfter}/${denAfter}`,
);
}
});

test('ratio - parse', t => {
const { brand: moeBrand } = makeIssuerKit('moe');
const { brand: larryBrand } = makeIssuerKit('larry');
Expand Down

0 comments on commit 9986152

Please sign in to comment.