Skip to content
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

feat(fast-usdc): withdraw and distribute contract fees #10815

Merged
merged 11 commits into from
Jan 15, 2025
Merged

Conversation

dckc
Copy link
Member

@dckc dckc commented Jan 8, 2025

closes: #10700
refs: #10847

Description

  • creatorFacet method to withdraw fees
    • unit test
  • core eval script to distribute fees
    • bootstrap test
    • refactor: publishDisplayInfo is not specific to fast-usdc

Security / Scaling Considerations / Upgrade Considerations

straightforward; not yet deployed

Documentation Considerations

  • builder script has usage docs: Use: [--fixedFees <number> | --feePortion <percent>] --destinationAddress <address>
  • document how to use it with cosgov

Testing Considerations

  • contract test: 1 positive, 1 negative
  • bootstrap test for core eval
    • fixed amount
    • portion
  • multichain test

@dckc dckc requested a review from turadg January 8, 2025 00:17
Copy link

cloudflare-workers-and-pages bot commented Jan 9, 2025

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: cfa9f78
Status: ✅  Deploy successful!
Preview URL: https://f048fcb9.agoric-sdk.pages.dev
Branch Preview URL: https://dc-fu-wd-fees.agoric-sdk.pages.dev

View logs

@dckc dckc changed the title feat(fast-usdc): fee recipient can withdraw fees feat(fast-usdc): withdraw and distribute contract fees Jan 10, 2025
@dckc dckc force-pushed the dc-fu-wd-fees branch 3 times, most recently from 9e95057 to 5446fa4 Compare January 13, 2025 20:57
@dckc dckc marked this pull request as ready for review January 13, 2025 20:57
@dckc dckc requested a review from a team as a code owner January 13, 2025 20:57
@dckc dckc removed the needs-design label Jan 13, 2025
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

Approving contingent on requests.

That multichain test turned out nicely

@@ -132,6 +140,7 @@ export const prepareLiquidityPoolKit = (zone, zcf, USDC, tools) => {
const poolStats = harden({
totalBorrows: makeEmpty(USDC),
totalContractFees: makeEmpty(USDC),
// TODO? availableContractFees: makeEmpty(USDC)
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary, please remove

Comment on lines 342 to 343
const proposal = seat.getProposal();
const { want } = proposal;
Copy link
Member

Choose a reason for hiding this comment

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

proposal isn't necessary and makes more for the reader to track.

Suggested change
const proposal = seat.getProposal();
const { want } = proposal;
const { want } = seat.getProposal();

then below,

zcf.atomicRearrange(harden([[feeSeat, seat, want]]));

Comment on lines +345 to +344
isGTE(available, want.USDC) ||
Fail`cannot withdraw ${want.USDC}; only ${available} available`;
Copy link
Member

Choose a reason for hiding this comment

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

out of scope but this strikes me as a useful helper fn

// COMMIT POINT
zcf.atomicRearrange(harden([[feeSeat, seat, proposal.want]]));
seat.exit();
// TODO? external.publishPoolMetrics();
Copy link
Member

Choose a reason for hiding this comment

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

nope

}) ||
assert.fail(usage)),
};
await writeCoreEval('eval-fast-usdc-distribute-fees', utils =>
Copy link
Member

Choose a reason for hiding this comment

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

consider matching the module name, fast-usdc-fees. If you prefer to include "distribute", it could also go in this module name. The "eval-" is common to every writeCoreEval so if you think that's worthwhile, we could put that into the helper but that's out of scope

Copy link
Member Author

Choose a reason for hiding this comment

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

This ci failure just reminded me why it should start with eval-:

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	multichain-testing/fast-usdc-fees-permit.json
	multichain-testing/fast-usdc-fees-plan.json
	multichain-testing/fast-usdc-fees.js

We have a .gitignore for it:

# builder prefix for core evals
eval-*

Copy link
Member

Choose a reason for hiding this comment

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

Oof, that's quite a surprise. No opinion on the name in this PR but I think we should not have module kinds designated by filename prefixes. We could have a suffix: #10490 (comment) or put the output in directory conventional for build output, like dist .

const kwUSDC = 'USDC'; // keyword in AmountKeywordRecord
const issUSDC = 'USDC'; // issuer name

const trace = (...args) => console.log('FUCF', ...args);
Copy link
Member

Choose a reason for hiding this comment

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

does importing makeTracer create problems? this is fine, just curious

Copy link
Member Author

Choose a reason for hiding this comment

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

no, I didn't run into problems.

This is left over from an earlier phase where I was avoiding all the builder stuff, since the core-eval is so simple. Then I realized the a3p core eval stuff expects it, so I stopped avoiding it.

@dckc
Copy link
Member Author

dckc commented Jan 13, 2025

I'm confused... this test runs fine locally...

p.s. not with SWINGSET_WORKER_TYPE=xs-worker

not ok 19 - fast-usdc › fast-usdc › distributes fees per BLD staker decision %ava-dur=6624ms
#   REJECTED from ava test.serial("distributes fees per BLD staker decision"): (Error#1)
#   Error#1: unsettled value for kp2318
#       at makeError (file:///home/runner/work/agoric-sdk/agoric-sdk/node_modules/ses/src/error/assert.js:350:61)
#       at fail (file:///home/runner/work/agoric-sdk/agoric-sdk/node_modules/ses/src/error/assert.js:482:20)
#       at Fail (file:///home/runner/work/agoric-sdk/agoric-sdk/node_modules/ses/src/error/assert.js:492:39)
#       at queueAndRun (file:///home/runner/work/agoric-sdk/agoric-sdk/packages/SwingSet/tools/run-utils.js:49:19)
#       at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
#       at async evalProposal (file:///home/runner/work/agoric-sdk/agoric-sdk/packages/boot/tools/supports.ts:618:5)
#       at async file:///home/runner/work/agoric-sdk/agoric-sdk/packages/boot/test/fast-usdc/fast-usdc.test.ts:398:5

@dckc
Copy link
Member Author

dckc commented Jan 14, 2025

ouch:

not ok 23 - fast-usdc › fast-usdc › replace operators %ava-dur=1ms
#   REJECTED from ava test.serial("replace operators"): (Error#2)
#   Error#2: unsettled value for kp2494

https://github.com/Agoric/agoric-sdk/actions/runs/12758495527/job/35560759090?pr=10815#step:5:7218

@dckc dckc added the force:integration Force integration tests to run on PR label Jan 14, 2025
@dckc dckc added the automerge:rebase Automatically rebase updates, then merge label Jan 14, 2025
@dckc dckc force-pushed the dc-fu-wd-fees branch 2 times, most recently from 61afe3a to 45dfb0a Compare January 15, 2025 00:35
Copy link
Contributor

mergify bot commented Jan 15, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

Copy link
Contributor

mergify bot commented Jan 15, 2025

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

1 similar comment
Copy link
Contributor

mergify bot commented Jan 15, 2025

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

Copy link
Contributor

mergify bot commented Jan 15, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

Copy link
Contributor

mergify bot commented Jan 15, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

@mergify mergify bot merged commit 44f449b into master Jan 15, 2025
81 checks passed
@mergify mergify bot deleted the dc-fu-wd-fees branch January 15, 2025 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ability to withdraw from Contract Fee account
2 participants