Skip to content

feat: throw SquidExecutionError with committed state - #27

Open
SgtPooki wants to merge 2 commits into
mainfrom
fix/execution-error-context
Open

feat: throw SquidExecutionError with committed state#27
SgtPooki wants to merge 2 commits into
mainfrom
fix/execution-error-context

Conversation

@SgtPooki

@SgtPooki SgtPooki commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

What changed

executeSquidFunding now throws SquidExecutionError when it fails after any transaction has been broadcast. The error carries completedRoutes (requirement IDs and hashes of finished legs), requirementId of the failed leg, transactionHash of the most recently broadcast transaction for that leg (an approval or the route itself), nativeFee committed to broadcast transactions so far, and the original error as cause. Before this, a failure in leg 2 of a multi-leg plan discarded leg 1's transaction hash, so a host could not tell what had already moved or record a rerun marker accurately. Failures before the first broadcast still throw plain errors, since nothing has changed on-chain.

The hash and fee are recorded at broadcast time, so a reverted transaction, a receipt failure, or a wallet rejection on a later transaction still reports exactly what went on-chain. The per-leg body moved into an executeQuote helper so the wrap happens at one call site; existing error messages are preserved inside the wrapper message, so callers matching on substrings keep working.

@FilOzzy FilOzzy added this to FOC Aug 19, 2026
@github-project-automation github-project-automation Bot moved this to 📌 Triage in FOC Aug 19, 2026
@SgtPooki SgtPooki self-assigned this Aug 19, 2026

@SgtPooki SgtPooki left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

self review

@SgtPooki
SgtPooki marked this pull request as ready for review August 20, 2026 17:34
@SgtPooki SgtPooki moved this from 📌 Triage to 🔎 Awaiting review in FOC Aug 20, 2026
@SgtPooki
SgtPooki requested a review from snissn August 20, 2026 17:34

@snissn snissn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@SgtPooki looks good!

I also had chatgpt pro do a review of the PR. Looks like it passes all of its checks , but it warns that there may be some correctness subtleties related to resolving conflicts when we make it mergeable

Codex review

I reviewed exact head 15976a1.

The committed-state design itself is coherent: it snapshots completed routes, preserves the underlying error as cause, distinguishes pre-broadcast failures from failures after an earlier broadcast, and records the latest transaction hash and cumulative fee only after sendTransaction returns. The new runtime export and tests are consistent, and the head CI run passed.

Blocking integration finding: this PR is still based on 5a6c7fd, while current main is 2493bfa; GitHub currently reports the PR as non-mergeable. The executor on current main has since gained SquidPriceQuote, maxNativeFee: bigint | "auto", native route-fee/value validation, and sourceDebit-aware balance accounting. Resolving the conflict toward this PR’s older executor would regress those safeguards.

Please rebase and transplant the committed-state machinery into the current executor while preserving the newer quote, native-value, fee-cap, and balance-accounting logic. In particular, retain the PR’s placement of totalNativeFee += prepared.fee after a successful sendTransaction return so a rejected, unbroadcast transaction is not reported as committed. I did not find an additional correctness problem in the proposed error-context logic itself.

Review generated by Codex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🔎 Awaiting review

Development

Successfully merging this pull request may close these issues.

3 participants