feat: fusdc advancer with fees #10494

0xpatrickdev · 2024-11-15T17:08:45Z

refs: #10390

Description

Add FeeConfig to privateArgs so deployers can adjust fees over time (terms are immutable)
Adds FeeTools for calculating net advance and fee splits
Integrates FeeTools with Advancer

Security Considerations

None really for these specific changes. We might need to think about timing from when the Advancer calls calculateAdvance and the Settler calls calculateSplit, as the FeeConfig values might change.
From a product POV, users might pay more in fees than the net advance they receive. So we might consider a config parameter for a "minimum request amount".

Scaling Considerations

None really, mainly contains AmountMath computation.

Documentation Considerations

Includes jsdoc and code comments

Testing Considerations

Includes unit tests for FeeTools, attempting to cover all foreseeable scenarios. A bit light on testing integrations with the advancer and settler.

Upgrade Considerations

N/A, unreleased

cloudflare-workers-and-pages · 2024-11-15T17:25:37Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`3771e7f`
Status:	✅ Deploy successful!
Preview URL:	https://d3aaff76.agoric-sdk.pages.dev
Branch Preview URL:	https://pc-fusdc-fees.agoric-sdk.pages.dev

View logs

github-actions · 2024-11-15T18:07:45Z

Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label.

turadg · 2024-11-15T18:22:07Z

packages/fast-usdc/src/fast-usdc.contract.js

 export const meta = {
  customTermsShape: {
-    contractFee: NatAmountShape,


ah yes, they should never have been terms because they must be changeable

turadg · 2024-11-15T18:22:39Z

packages/fast-usdc/src/fast-usdc.contract.js

@@ -53,6 +50,7 @@ export const contract = async (zcf, privateArgs, zone, tools) => {
  const terms = zcf.getTerms();
  assert('USDC' in terms.brands, 'no USDC brand');
  assert('usdcDenom' in terms, 'no usdcDenom');
+  mustMatch(privateArgs.feeConfig, FeeConfigShape, 'must provide feeConfig');


please instead use meta.privateArgsShape

turadg · 2024-11-15T18:23:12Z

packages/fast-usdc/src/fast-usdc.contract.js

@@ -64,6 +62,7 @@ export const contract = async (zcf, privateArgs, zone, tools) => {
  const { chainHub, vowTools } = tools;
  const makeAdvancer = prepareAdvancer(zone, {
    chainHub,
+    feeConfig: privateArgs.feeConfig,


nit: cleaner to destructure in the fn params

turadg · 2024-11-15T18:25:52Z

packages/fast-usdc/src/utils/fees.js

+     * @param {Amount<'nat'>} requested must be greater than 0
+     */
+    calculateAdvanceFee(requested) {
+      !isEqual(requested, emptyAmount) || Fail`Fee exceeds requested.`;


This will error "Fee exceeds" if the brands don't match. The check should include brands. Consider instead a nonZeroAmount shape made using the limits param. Then this could handle both checks cleanly.

Suggested change

!isEqual(requested, emptyAmount) || Fail`Fee exceeds requested.`;

mustMatch(requested, nonZeroAmount);

Good feedback. Just now I made NonZeroAmountShape = { brand: BrandShape, value: M.gte(1n) } and implemented mustMatch. I made it brand agnostic as the consuming services will have done this check already.

In light of Dan's feedback, I came to realize this check was vestigial and have removed it. Some iteration of this couldn't deal with zeros during math.

turadg · 2024-11-15T18:28:20Z

packages/fast-usdc/test/utils/fees.test.ts

+  };
+};
+
+const feeToolsScenario = test.macro({


perfect use of macros

dckc

this is how far I got before meetings...

dckc · 2024-11-15T17:37:34Z

packages/fast-usdc/src/fast-usdc.contract.js

-    contractFee: NatAmountShape,
-    poolFee: NatAmountShape,


taking these out of the terms is an unfortunate regression. I suppose I understand the motivation - they can change on restart / upgrade.

Plus, our clients can't do getTerms(). They can only read vstorage....

Yes the ability to change fees is the main motivation here. We could consider putting initialFeeConfig in terms, but it seems superfluous to me.

dckc · 2024-11-15T17:38:40Z

packages/fast-usdc/src/fast-usdc.contract.js

@@ -53,6 +50,7 @@ export const contract = async (zcf, privateArgs, zone, tools) => {
  const terms = zcf.getTerms();
  assert('USDC' in terms.brands, 'no USDC brand');
  assert('usdcDenom' in terms, 'no usdcDenom');
+  mustMatch(privateArgs.feeConfig, FeeConfigShape, 'must provide feeConfig');


let Zoe do this: put FeeConfigShape and such in meta.privateArgsShape.

dckc · 2024-11-15T17:41:30Z

packages/fast-usdc/src/typeGuards.js

+  flat: NatAmountShape,
+  variableRate: RatioShape,
+  maxVariable: NatAmountShape,
+  contractRate: RatioShape,


Ideally, we'd constrain the brand in all these. But we want this shape in meta.privateArgsShape, where we don't have the brand identity.

dckc · 2024-11-15T17:43:41Z

packages/fast-usdc/src/utils/fees.js

+     * @param {Amount<'nat'>} requested
+     */
+    calculateAdvance(requested) {
+      const fee = this.calculateAdvanceFee(requested);


We don't use this except in exo's. (IOU pointer to docs)
Please define separate functions and then return a hardened record of them.

Or do const self = harden({ ... }); return self; and use self.calculateAdvanceFee. Or tools.

dckc · 2024-11-15T17:45:04Z

packages/fast-usdc/src/utils/fees.js

+     * @param {Amount<'nat'>} requested must be greater than 0
+     */
+    calculateAdvanceFee(requested) {
+      !isEqual(requested, emptyAmount) || Fail`Fee exceeds requested.`;


Suggested change

!isEqual(requested, emptyAmount) || Fail`Fee exceeds requested.`;

!isEmpty(requested) || Fail`Fee exceeds requested.`;

Why the special case, though?

TIL isEmpty, although it was right in front of my face, that's handy.

why the special case

You're right, no need for this. This is vestigial from when some iteration couldn't deal with zeros.

dckc · 2024-11-15T18:20:37Z

packages/fast-usdc/src/exos/advancer.js

@@ -123,14 +126,17 @@ export const prepareAdvancerKit = (
            // this will throw if the bech32 prefix is not found, but is handled by the catch
            const destination = chainHub.makeChainAddress(EUD);
            const requestedAmount = toAmount(evidence.tx.amount);
+            const advanceAmount = feeTools.calculateAdvance(requestedAmount);


likewise, add a @throws for calculateAdvance, please.

dckc · 2024-11-15T18:23:46Z

packages/fast-usdc/src/exos/advancer.js

+              // report `requestedAmount`, not `advancedAmount`... do we need to
+              // communicate net to `StatusManger` in case fees change in between?


I suggest adding a test to answer this question -- a test to simulate a dashboard query on output of the StatusManager. It's possible that the dashboard requirements aren't sufficiently clear, but this would be a good form of review.

simulate a dashboard query

Agree, but we have a "Status Manager writes to vstorage" task scheduled for the future. We can comprehensively tackle this then.

P.S. - this particular question is more about settling with the same fee split we used during advance. I agree we should have a test for this, but the settler is not wired up to use the FeeConfig yet.

scheduled for the future

yes; my review was rushed and I didn't manage to separate things in scope for this PR from other thinking-out-loud.

does a test.todo() seem cost-effective?

dckc · 2024-11-15T18:27:41Z

packages/fast-usdc/src/exos/advancer.js

@@ -147,7 +153,7 @@ export const prepareAdvancerKit = (
            }

            try {
-              const payment = await assetManagerFacet.borrow(requestedAmount);
+              const payment = await assetManagerFacet.borrow(advanceAmount);


just below this, I see poolAccount. Its type is ERef<...>. Do we test the promise case? I don't think promises can be stored durably.

I expect poolAccount to be a vow or a remotable. The former can be stored durably.

Look for tests in #10497, or further in the future if scope is cut, as the contract doesn't currently create an LOA.

dckc · 2024-11-15T18:28:46Z

packages/fast-usdc/src/fast-usdc.contract.js

@@ -44,6 +40,7 @@ harden(meta);
 * @param {ZCF<FastUsdcTerms>} zcf
 * @param {OrchestrationPowers & {
 *   marshaller: Marshaller;
+ *   feeConfig: FeeConfig;


Let's add this to publicTopics to be sure vstorage clients can see them.

We have a future task "Write contract config to vstorage" where we can tackle this. I'd add a todo to the tasklist, but it's still a draft.

dckc · 2024-11-15T18:37:06Z

packages/fast-usdc/test/mocks.ts

+    variableRate: makeRatio(2n, usdc.brand, 100n, usdc.brand),
+    maxVariable: usdc.units(100),
+    contractRate: makeRatio(2n, usdc.brand, 10n, usdc.brand),


The denominator brand defaults to the numerator brand.
And the denominator defaults to 100n.

Suggested change

variableRate: makeRatio(2n, usdc.brand, 100n, usdc.brand),

maxVariable: usdc.units(100),

contractRate: makeRatio(2n, usdc.brand, 10n, usdc.brand),

variableRate: makeRatio(2n, usdc.brand),

maxVariable: usdc.units(100),

contractRate: makeRatio(20n, usdc.brand),

Good tip, thanks

dckc

yup. let's do this.

dckc · 2024-11-15T23:00:22Z

packages/fast-usdc/src/type-guards.js

+
+/** @type {TypedPattern<string>} */
+export const EvmHashShape = M.string({
+  stringLengthLimit: 66,


interesting... We don't have patterns for "starts with xyz" and the like, so I had only thought of string length limits as denial-of-service protection.

turadg · 2024-11-15T23:11:15Z

packages/fast-usdc/src/fast-usdc.contract.js

-const NatAmountShape = { brand: BrandShape, value: M.nat() };
-export const meta = {
+
+export const meta = /** @type {ContractMeta<FastUsdcSF>} */ ({


how is this better than the existing convention?

Suggested change

export const meta = /** @type {ContractMeta<FastUsdcSF>} */ ({

/** @type {ContractMeta<typeof start>} */

turadg · 2024-11-15T23:14:57Z

packages/fast-usdc/src/utils/fees.js

+    calculateAdvanceFee(requested) {
+      const variable = min(multiplyBy(requested, variableRate), maxVariable);
+      const fee = add(variable, flat);
+      !isGTE(fee, requested) || Fail`Fee exceeds requested.`;


If fee == requested then isGTE will be true, so !isGTE will be false and the Fail will throw.

Suggested change

!isGTE(fee, requested) || Fail`Fee exceeds requested.`;

isGTE(requested, fee) || Fail`Fee exceeds requested.`;

Or,

Suggested change

!isGTE(fee, requested) || Fail`Fee exceeds requested.`;

!isGTE(fee, requested) || Fail`Request must exceed fees.`;

If the latter, updates the @throws too.

Updated the error message and @throws

- useful for `privateArgsShape` in consuming contracts - renames cosmos `orchestrationService`s powers from `OrchestrationPowers` -> `CosmosOrchestrationPowers`

0xpatrickdev force-pushed the pc/fusdc-advancer branch from 165b3d7 to 99707ef Compare November 15, 2024 17:20

0xpatrickdev force-pushed the pc/fusdc-fees branch from 8fcf7cb to 06ca4dd Compare November 15, 2024 17:25

0xpatrickdev marked this pull request as ready for review November 15, 2024 17:28

0xpatrickdev requested a review from a team as a code owner November 15, 2024 17:28

0xpatrickdev requested review from AgoricTriage, turadg and dckc November 15, 2024 17:28

Base automatically changed from pc/fusdc-advancer to master November 15, 2024 18:07

0xpatrickdev force-pushed the pc/fusdc-fees branch from 06ca4dd to 31a6f68 Compare November 15, 2024 18:09

dckc mentioned this pull request Nov 15, 2024

feat(fast-usdc): core-eval, builder script #10477

Merged

3 tasks

turadg reviewed Nov 15, 2024

View reviewed changes

dckc reviewed Nov 15, 2024

View reviewed changes

0xpatrickdev force-pushed the pc/fusdc-fees branch 4 times, most recently from 3ad5d52 to a1d931d Compare November 15, 2024 20:23

0xpatrickdev requested review from turadg and dckc November 15, 2024 22:20

dckc approved these changes Nov 15, 2024

View reviewed changes

turadg requested changes Nov 15, 2024

View reviewed changes

0xpatrickdev added 2 commits November 18, 2024 09:35

feat: export OrchestrationPowersShape

34b61ea

- useful for `privateArgsShape` in consuming contracts - renames cosmos `orchestrationService`s powers from `OrchestrationPowers` -> `CosmosOrchestrationPowers`

refactor: single type-guards.js file

72f34fc

0xpatrickdev force-pushed the pc/fusdc-fees branch from a1d931d to cd0c2bf Compare November 18, 2024 14:36

0xpatrickdev added 2 commits November 18, 2024 10:02

feat: advancer with fees

087f3a8

types: throws jsdoc annotation

3771e7f

0xpatrickdev force-pushed the pc/fusdc-fees branch from cd0c2bf to 3771e7f Compare November 18, 2024 15:02

0xpatrickdev requested a review from turadg November 18, 2024 15:23

turadg approved these changes Nov 18, 2024

View reviewed changes

0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Nov 18, 2024

mergify bot merged commit 7390224 into master Nov 18, 2024
90 of 91 checks passed

mergify bot deleted the pc/fusdc-fees branch November 18, 2024 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fusdc advancer with fees #10494

feat: fusdc advancer with fees #10494

0xpatrickdev commented Nov 15, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 15, 2024 •

edited

Loading

github-actions bot commented Nov 15, 2024

turadg Nov 15, 2024

turadg Nov 15, 2024

turadg Nov 15, 2024

turadg Nov 15, 2024

0xpatrickdev Nov 15, 2024

turadg Nov 15, 2024

dckc left a comment

dckc Nov 15, 2024

0xpatrickdev Nov 15, 2024

dckc Nov 15, 2024

dckc Nov 15, 2024

dckc Nov 15, 2024

dckc Nov 15, 2024

0xpatrickdev Nov 15, 2024

dckc Nov 15, 2024

dckc Nov 15, 2024

0xpatrickdev Nov 15, 2024 •

edited

Loading

dckc Nov 15, 2024

dckc Nov 15, 2024

0xpatrickdev Nov 15, 2024

dckc Nov 15, 2024

0xpatrickdev Nov 15, 2024

dckc Nov 15, 2024

dckc Nov 15, 2024

0xpatrickdev Nov 15, 2024

dckc left a comment

dckc Nov 15, 2024

turadg Nov 15, 2024

turadg Nov 15, 2024

0xpatrickdev Nov 18, 2024

	!isEqual(requested, emptyAmount) \|\| Fail`Fee exceeds requested.`;
	mustMatch(requested, nonZeroAmount);

	!isEqual(requested, emptyAmount) \|\| Fail`Fee exceeds requested.`;
	!isEmpty(requested) \|\| Fail`Fee exceeds requested.`;

		// report `requestedAmount`, not `advancedAmount`... do we need to
		// communicate net to `StatusManger` in case fees change in between?

	export const meta = /** @type {ContractMeta<FastUsdcSF>} */ ({
	/** @type {ContractMeta<typeof start>} */

	!isGTE(fee, requested) \|\| Fail`Fee exceeds requested.`;
	isGTE(requested, fee) \|\| Fail`Fee exceeds requested.`;

	!isGTE(fee, requested) \|\| Fail`Fee exceeds requested.`;
	!isGTE(fee, requested) \|\| Fail`Request must exceed fees.`;

feat: fusdc advancer with fees #10494

feat: fusdc advancer with fees #10494

Conversation

0xpatrickdev commented Nov 15, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Nov 15, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

github-actions bot commented Nov 15, 2024

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

dckc 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

0xpatrickdev Nov 15, 2024 • edited Loading

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

dckc 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

0xpatrickdev commented Nov 15, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Nov 15, 2024 •

edited

Loading

0xpatrickdev Nov 15, 2024 •

edited

Loading