chore: provisionSmartWallet flake
#10691
Conversation
0xpatrickdev
requested review from
AgoricTriage,
turadg,
dckc and
samsiegart
Deploying agoric-sdk with
|
| Latest commit: |
0cdb981
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://772aea64.agoric-sdk.pages.dev |
| Branch Preview URL: | https://pc-provision-sw-flake.agoric-sdk.pages.dev |
| @@ -28,11 +28,11 @@ const retryUntilCondition = async <T>( | |||
| { | |||
| maxRetries = 6, | |||
| retryIntervalMs = 3500, | |||
| log = () => {}, | |||
| log = console.log, | |||
There was a problem hiding this comment.
personally I find this log output noisy. Good to change console.log below to this log, but consider leaving the noop default for silence
There was a problem hiding this comment.
This (+ console.log -> log) would impact a lot of existing tests, so I chose to preserve the existing behavior. I'll see how this looks in local runs and will look to land it in the future:
+ log = () => {},
- log = console.log,
0xpatrickdev
added
automerge:rebase
| // execute sequentially, to avoid "published.wallet.${addr}.current: fetch failed" | ||
| const oracleWds: WalletDriver[] = []; | ||
| for (const p of oracleWdPs) { | ||
| const wd = await p; |
There was a problem hiding this comment.
yeah... that never did execute sequentially; that is: it didn't wait for one to finish before starting the next.
There was a problem hiding this comment.
Oh right, they all start together
multichain-testing/test/support.ts
Outdated
| @@ -79,13 +79,13 @@ export const commonSetup = async (t: ExecutionContext) => { | |||
| console.error('setupRegistry failed', e); | |||
| throw e; | |||
| } | |||
| const tools = await makeAgdTools(t.log, childProcess); | |||
| const keyring = await makeKeyring(tools); | |||
| const deployBuilder = makeDeployBuilder(tools, fse.readJSON, execa); | |||
| const retryUntilCondition = makeRetryUntilCondition({ | |||
| log: t.log, | |||
| setTimeout: globalThis.setTimeout, | |||
There was a problem hiding this comment.
only top level scripts (incl. test scripts) should be grabbing ambient authority like this use of globalThis.setTimeout.
pre-dates this PR, though, so I'll add it to the list:
There was a problem hiding this comment.
I thought we agreed modules that are used only in testing can have ambient authority?
(btw, I bristle at the subjectivity and emotional valence of that issue title. s/Distressing //)
multichain-testing/test/support.ts
Outdated
| const retryUntilCondition = makeRetryUntilCondition({ | ||
| log: t.log, | ||
| setTimeout: globalThis.setTimeout, | ||
| }); | ||
| const tools = await makeAgdTools(t.log, childProcess, retryUntilCondition); |
There was a problem hiding this comment.
though... now tools and everything derived from it carries the ambient authority along.
childProcess was already ambient authority though.
| () => q.queryData(`published.wallet.${address}.current`), | ||
| result => !!result, | ||
| `wallet in vstorage ${address}`, | ||
| { log: () => {} }, // suppress logs as this is already noisy |
There was a problem hiding this comment.
yeah; we do too much logging without giving the caller the ability to turn it off.
| setTimeout, | ||
| rpcAddress = 'http://localhost:26657', | ||
| apiAddress = 'http://localhost:1317', | ||
| retryUntilCondition, |
There was a problem hiding this comment.
setTimeout is already in the list... I suppose somebody might want the log in retryUntilCondition to be different from the log passed to makeE2ETools, but in the common case, we can fill it in for them:
| setTimeout, | |
| rpcAddress = 'http://localhost:26657', | |
| apiAddress = 'http://localhost:1317', | |
| retryUntilCondition, | |
| setTimeout, | |
| rpcAddress = 'http://localhost:26657', | |
| apiAddress = 'http://localhost:1317', | |
| retryUntilCondition = makeRetryUntilCondition({ log, setTimeout }), |
There was a problem hiding this comment.
Nice, good thinking
0xpatrickdev
removed
the
automerge:rebase
|
This pull request has been removed from the queue for the following reason: Pull request #10691 has been dequeued. The pull request rule doesn't match anymore. The following conditions don't match anymore:
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: |
- motivated after observing "cannot read data of published.wallet.agoric1ujmk0492mauq2f2vrcn7ylq3w3x55k0ap9mt2p.current: fetch failed" in https://github.com/Agoric/agoric-sdk/actions/runs/12313012295/job/34369641775?pr=10638 - this likely indicates a race between `provision-one`, the vstorage update, or the RPCs view of vstorage - to address this, retry the vstorage query with `retryUntilCondition`
0xpatrickdev
force-pushed
the
pc/provision-sw-flake
branch
from
0daf237 to
0cdb981
Compare
Description
cannot read data of published.wallet.${addr}.current: fetch failedin 10638Security Considerations
None
Scaling Considerations
None
Documentation Considerations
None
Testing Considerations
None
Upgrade Considerations
None