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

6000 remove zoe floating promises #9458

Merged
merged 9 commits into from
Jun 7, 2024
Merged

Conversation

Chris-Hibbert
Copy link
Contributor

closes: #6000

Description

Resolve remaining floating promises in Zoe

Security Considerations

Zoe's correct functioning is crucial to the chain. I determined that it was correct to not further handle any of these promises.

Scaling Considerations

N/A

Documentation Considerations

N/A

Testing Considerations

N/A

Upgrade Considerations

No impact, because no behavioral changes.

@Chris-Hibbert Chris-Hibbert added Zoe package: Zoe hygiene Tidying up around the house code-style defensive correctness patterns; readability thru consistency labels Jun 5, 2024
@Chris-Hibbert Chris-Hibbert self-assigned this Jun 5, 2024
Copy link

cloudflare-workers-and-pages bot commented Jun 6, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 87268ad
Status: ✅  Deploy successful!
Preview URL: https://f74218b7.agoric-sdk.pages.dev
Branch Preview URL: https://6000-no-zoe-floating-promise.agoric-sdk.pages.dev

View logs

@Chris-Hibbert Chris-Hibbert requested a review from turadg June 7, 2024 16:28
@Chris-Hibbert
Copy link
Contributor Author

@turadg: please check out my revisions to the tests, particularly priceAggregator test. The commits segregate the changes cleanly.

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.

It's a relief to see that the only awaits necessary were in test code. =)

@@ -42,7 +41,7 @@ const build = async (log, zoe, issuers, payments, installations) => {
});
const alicePayments = { Asset: moolaPayment };

logCounter(log, publicFacet);
await logCounter(log, publicFacet);
Copy link
Member

Choose a reason for hiding this comment

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

probably ok if these stayed void but good to have the confident now of what the order is

@@ -732,15 +737,20 @@ test('quoteWhen', async t => {

/** @type {PriceQuote | undefined} */
let abovePriceQuote;
t.notThrowsAsync(quoteWhenGTE.then(result => (abovePriceQuote = result)));
const results = [];
Copy link
Member

Choose a reason for hiding this comment

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

consider commenting here why this array is necessary

@Chris-Hibbert Chris-Hibbert force-pushed the 6000-no-zoe-floating-promises branch from c012f04 to 3ffdfe8 Compare June 7, 2024 17:32
@Chris-Hibbert Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Jun 7, 2024
@Chris-Hibbert Chris-Hibbert force-pushed the 6000-no-zoe-floating-promises branch from 3ffdfe8 to 87268ad Compare June 7, 2024 17:56
@mergify mergify bot merged commit 3154483 into master Jun 7, 2024
63 checks passed
@mergify mergify bot deleted the 6000-no-zoe-floating-promises branch June 7, 2024 18:30
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 code-style defensive correctness patterns; readability thru consistency hygiene Tidying up around the house Zoe package: Zoe
Projects
None yet
Development

Successfully merging this pull request may close these issues.

resolve all floating promise warnings
2 participants