fix(wallet): hireErc8183Agent throws a false "job is not ours" error when opts.noWait is set - #42
Conversation
…batch unconfirmed
execute() returns status PENDING immediately after the relay accepts the
intent when opts.noWait is set, before the batch is mined. hireErc8183Agent's
post-funding check read getErc8183Job right after that, so it observed
pre-inclusion chain state and threw "job ... is not ours after funding" on
every noWait call regardless of whether the funding would go on to succeed.
Only run the check once execute() reports CONFIRMED. The default (waiting)
path is unchanged.
Adds coverage in sessionKeyRegistration.test.ts, reusing its existing
relay-boundary mock rather than adding a second mock.module("./internal/relay.js")
registration in a new file (that leaked into client.balances.test.ts when tried).
|
@web3xDev is attempting to deploy a commit to the Functor Team on Vercel. A member of the Team first needs to authorize it. |
dhernz
left a comment
dhernz
left a comment
There was a problem hiding this comment.
Thank you for this — a genuinely excellent first contribution, and welcome!
We verified the report end-to-end before merging, and everything checks out:
- Bug confirmed in source:
execute()withnoWait: truereturns{status: "PENDING"}before the batch is mined, andhireErc8183Agent's post-funding check ran unconditionally against pre-inclusion state — so everynoWaithire threw the false "job is not ours" error, with the double-escrow retry hazard you described. - Tests proven meaningful: we applied only your test file to the unpatched branch — the noWait test fails there with exactly the reported error, and passes with your fix. The default-path mismatch protection still throws as intended.
- Full gates green on your branch: typecheck, build, and all wallet test files pass per-file.
Also appreciated the care around bun's shared module registry — your reasoning for placing the tests in sessionKeyRegistration.test.ts matches a quirk we've documented in our CI config.
One housekeeping note: we've retargeted this PR to staging, which is now the repo's default branch. Our flow is feature branches → staging (accumulates the next SDK release) → main (the published SDK), so your fix will ship with the next version publish. New CONTRIBUTING.md documents this for next time. Hope to see you around!
- CHANGELOG.md: Fixed entry under [Unreleased] for the hireErc8183Agent noWait false-error fix (credits @web3xDev, first external contribution). - CONTRIBUTING.md: user-visible changes require an [Unreleased] entry; document the publish step (rename to version+date, publish to npm, merge staging -> main). - PR template: changelog checkbox.
|
Thanks so much for the kind words and for taking the time to verify everything! Really glad the fix and tests checked out. Looking forward to contributing more! |
2 participants
Bug
hireErc8183Agent's post-funding check reads getErc8183Job immediately after execute() returns to confirm that the predicted jobId actually belongs to the caller.
With opts.noWait: true, execute() returns { status: "PENDING" } as soon as the relay accepts the intent, before the batch is mined. The post-check didn't account for this: it read the pre-inclusion chain state and threw "job is not ours after funding" on every noWait call, even when the funding would succeed a few seconds later.
No test or e2e script in the repo covered hireErc8183Agent with noWait: true.
If a caller retries on that error, it re-runs the entire hire batch and escrows $U a second time for a job that was already successfully funded.
Fix
Only run the post-check when execute() reports CONFIRMED. The default waiting behavior is unchanged. Also updated the docstring, which claimed the function "returns once the job is FUNDED on-chain" — that is no longer true for the noWait path.
Testing
Added 3 tests covering the noWait: true and default paths. The noWait test verifies that the post-check is skipped; the default-path tests verify that a real mismatch still throws and a matching job still succeeds.
The tests were added to sessionKeyRegistration.test.ts instead of a new file because bun shares one module registry across test files. A second mock.module("./internal/relay.js") registration in a separate file leaked into client.balances.test.ts when I tried that approach.
bun test: 74 pass, 0 fail.
bun run typecheck: clean.
bun run build: clean.