fix(wallet): report per-asset faucet outcomes - #35
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e02e445ca0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
snissn
left a comment
snissn
left a comment
There was a problem hiding this comment.
Codex review — changes required before merge
Reviewed on exact head e02e445ca09d29c67eef48bc41fbc9a3691b99a7.
Three correctness/UX issues remain:
- The command treats an absolute nonzero balance as proof that this faucet attempt funded the asset. A pre-funded wallet can therefore turn a rejected, reverted, or timed-out claim into
status: funded, with the failure relegated tofaucetError/errorand the success CTA emitted. Capture pre-claim balances and report whether the balance increased, or distinguishalready_fundedreadiness from the outcome of this invocation. Add regression coverage for a failed/reverted claim against pre-existing positive balances. The existing unresolved inline thread identifies this correctly. - A successful receipt followed by one zero balance read is classified as
missingand immediately recommends another faucet. That can duplicate a claim when the balance read is lagging. A submitted/confirmed transaction without an observed increase should remainunconfirmeduntil a bounded recheck establishes the result; only a definitively unsubmitted or failed asset should be eligible for the fallback CTA. - The fully funded CTA still hard-codes
wallet deposit 1. That bypasses the actual file size, runway, copy count, CDN choice, existing Filecoin Pay funds, anddepositNeeded. Remove the fixed deposit recommendation and direct the agent to cost the intended upload first; no deposit amount should be suggested until that quote exists.
The independent receipt waits, mainnet preflight rejection, and structured partial/unconfirmed output are good directions, but the current status and CTA contract can still produce a false success or an unnecessary fund-moving action.
GitHub does not permit a reviewer authenticated as the PR author to submit a formal REQUEST_CHANGES review, so this is posted as a blocking comment review.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR updates the wallet fund command to report per-asset (FIL/USDFC) faucet outcomes with richer status semantics, ensuring funding is only reported as successful when both a successful receipt and an observed positive balance are present, and updating documentation/tests accordingly.
Changes:
- Return structured per-asset results (
funded/missing/unconfirmed) plus a top-level status instead of collapsing outcomes into a single failure. - Await and evaluate each faucet transaction receipt, then re-check balances to confirm funding.
- Update troubleshooting documentation and extend command tests/mocks to cover partial, timeout, revert, and helper-failure cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| skills/foc-cli/references/troubleshooting.md | Documents the new wallet fund result contract and clarifies TESTNET_ONLY behavior. |
| cli/tests/synapse-commands.test.ts | Adds/updates tests for per-asset outcomes, unconfirmed behavior, and mainnet rejection. |
| cli/tests/command-mocks.ts | Updates faucet claim mocks to return per-asset tx hashes. |
| cli/src/commands/wallet/fund.ts | Implements per-asset outcome tracking, receipt waiting, balance confirmation, and structured output/CTA logic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5b7ed2b20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Resolved on current head
The follow-up exact-head Codex review found one adjacent edge case, fixed in |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 261264be51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b2902ecf2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Final review convergence is complete on The three GPT Pro notes remain addressed. Subsequent Codex/Copilot findings on definitive revert preservation, unknown faucet metadata, and truthful process-log failure states were also fixed, answered inline, and resolved. Validation is 212 tests / 533 assertions plus Biome, TypeScript, build, exact-head CI, and a clean exact-head Codex review. GitHub reports zero unresolved review threads. The PR remains open and unmerged. |
Summary
funded.unconfirmedand require a balance recheck before retrying.wallet costsfor the intended upload instead of recommending a fixed deposit.Review convergence
Two independent internal reviews covered correctness and simplification. Their findings on timeout safety, top-level status precedence, duplicate claim guidance, documentation, and duplicated types were fixed; both final re-reviews were clean.
Codex found that an absolute positive balance could overwrite a failed faucet outcome. The promotion rule is now restricted to successful receipts, with focused coverage for helper failure, timeout, and revert cases. GPT Pro then identified a delayed-balance retry risk and the fixed deposit CTA; successful receipts with zero observed balance now remain unconfirmed, and the CTA requires upload-specific costing before any deposit. Follow-up reviews found that a failed balance lookup could overwrite a definitive revert, unknown faucet metadata could be assigned to USDFC, and failure paths left contradictory successful process-log steps. Each outcome and failed stage is now classified explicitly, and all corresponding threads are resolved.
Validation
bun run test: 212 passed, 533 assertionsbunx biome check src testsbunx tsc --noEmitbun run buildCompatibility note
The command now returns structured per-asset results for partial, missing, and unconfirmed funding instead of collapsing faucet failures into
FUND_FAILED. The upstream helper discards per-asset details when it throws, so those cases are truthfully reported as unconfirmed rather than attributed speculatively.