search

Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
327 stars 207 forks source link

fix(orchestration): subscribeToTransfers() atomically #10553

Closed dckc closed 23 hours ago

dckc commented 4 days ago

refs: #10391

Description

There was a race in subscribeToTransfers. Use a Vow to avoid the race.

Security / Scaling / Upgrade / Documentation Considerations

can't think of any

Testing Considerations

I'm not sure how to make a test for this. The purpose of this fix is mostly to stop other tests from failing.

0xpatrickdev commented 4 days ago

These are two tests we have that cover this path (via auto-stake-it.contract.js / auto-stake-it.flows.js) :

I assume they don't have the same symptoms we are currently facing due to these async calls in between .makeAccount() and .monitorTransfer(): https://github.com/Agoric/agoric-sdk/blob/0105e1ac69b66895872c2e3243f8162069491208/packages/orchestration/src/examples/auto-stake-it.flows.js#L45-L78

But their passing should help towards giving us confidence this change does not introduce a regression.

cloudflare-workers-and-pages[bot] commented 4 days ago

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7b77993
Status: ✅  Deploy successful!
Preview URL: https://da154411.agoric-sdk.pages.dev
Branch Preview URL: https://dc-monitor-race.agoric-sdk.pages.dev

View logs

michaelfig commented 2 days ago

I think we need @iomekam or @michaelfig who I think made packet-tools.

Guilty as charged! (Thanks for the ping, I'm taking a look now.)