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

fix(fast-usdc): .exit() all internal temporary seats #10939

Merged
merged 5 commits into from
Feb 7, 2025
Merged

Conversation

dckc
Copy link
Member

@dckc dckc commented Feb 4, 2025

closes #10890

refs:

Description

mainly:

  • fix: .exit() all internal temporary seats

Also, to exercise pruning vstorage:

  • feat: core eval script to deleteCompletedTxs
    • expose deleteCompletedTx via creatorFacet

To speed up testing and match it with expected deployment:

  • chore: don't write-chain-info when testing Fast USDC

Plus a misc type refinement:

  • chore: refine return type of Offers.deposit

Security / Scaling Considerations

Reducing leaks improves scalability which improves availability.

Documentation Considerations

I don't think so.

Testing Considerations

Tests are in #10931 ; they're somewhat rough and not included in this PR.

Upgrade Considerations

Some undiagnosed heap growth remains. A fix for that might go out in a future upgrade.

@dckc dckc requested a review from turadg February 4, 2025 18:12
Copy link

cloudflare-workers-and-pages bot commented Feb 4, 2025

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0b961a1
Status: ✅  Deploy successful!
Preview URL: https://43632cff.agoric-sdk.pages.dev
Branch Preview URL: https://dc-fu-res-misc.agoric-sdk.pages.dev

View logs

};

trace('script finishing');
pruneFastUsdcStorage;
Copy link
Member

Choose a reason for hiding this comment

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

We don't have an extension for scripts that run in CoreEval. .core.js is for scripts that are built by a .build.js into a script that runs.

So I think this filename is fine for now, but it shouldn't be part of src because it's not a package export. How about in scripts?

@@ -223,7 +223,8 @@ export const prepareAdvancerKit = (
onFulfilled(result, ctx) {
const { poolAccount, intermediateRecipient, settlementAddress } =
this.state;
const { destination, advanceAmount, tmpSeat: _, ...detail } = ctx;
const { destination, advanceAmount, tmpSeat, ...detail } = ctx;
tmpSeat.exit();
Copy link
Member

Choose a reason for hiding this comment

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

👍

@dckc dckc force-pushed the dc-fu-res-misc branch 5 times, most recently from c757f81 to d244bd0 Compare February 6, 2025 23:17
@dckc dckc marked this pull request as ready for review February 6, 2025 23:23
@dckc dckc requested a review from a team as a code owner February 6, 2025 23:23
@dckc dckc changed the title fix(fast-usdc): reduce leaks etc. fix(fast-usdc): .exit() all internal temporary seats Feb 6, 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.

Nice

@dckc dckc added the automerge:rebase Automatically rebase updates, then merge label Feb 6, 2025
Copy link
Contributor

mergify bot commented Feb 7, 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 Feb 7, 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 Feb 7, 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

@dckc
Copy link
Member Author

dckc commented Feb 7, 2025

Even after 3 tries, I'm blocked by a ci flake

Copy link
Contributor

mergify bot commented Feb 7, 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 7f8ede3 into master Feb 7, 2025
83 checks passed
@mergify mergify bot deleted the dc-fu-res-misc branch February 7, 2025 16:44
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fast USDC load test
3 participants