Replace Faucet fixed-sleep settle with eth_getBalance poll loop#1
Merged
Conversation
📝 WalkthroughWalkthroughThis PR replaces fixed-delay settlement in ChangesPolling-based wallet settlement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Replaces the fixed 2.5 s Process.sleep in Onchain.Tempo.Faucet.fresh_funded_wallet/1 with an eth_getBalance poll loop that returns as soon as the funding transaction lands on-chain. :settle_ms is repurposed as the poll timeout (default 10_000 ms), and a new :poll_interval_ms option (default 200 ms) controls the cadence. Roadmap/changelog metadata is updated accordingly.
Changes:
- Refactor
fresh_funded_wallet/1to usewith+wait_for_funding/2that pollseth_getBalanceuntil non-zero or deadline reached. - Update module/option docstrings and add two new unit tests (poll-success and poll-timeout) using
Req.Teststubs and:counters. - Mark Task 6 done in
roadmap/tasks.toml,roadmap/data.json,ROADMAP.md; add a CHANGELOG entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/onchain/tempo/faucet.ex | Replaces fixed sleep with bounded eth_getBalance poll loop; adds parse_balance_hex/1 and poll_balance/6; updates docs. |
| test/onchain/tempo/faucet_test.exs | Adds two tests covering successful polling and timeout behavior. |
| roadmap/tasks.toml | Marks Task 6 status as done. |
| roadmap/data.json | Mirrors Task 6 status update. |
| ROADMAP.md | Flips Task 6 row to ✅. |
| CHANGELOG.md | Documents the new polling behavior and option semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/onchain/tempo/faucet.ex`:
- Around line 103-112: The code currently reads :settle_ms into timeout_ms and
:poll_interval_ms into interval_ms then calls poll_balance(addr_hex, url,
rpc_opts, deadline, interval_ms, timeout_ms) without validating bounds; add
input validation at the start of the function that reads opts (before computing
deadline/poll_balance): ensure :settle_ms (timeout_ms) is an integer >= 0 and
:poll_interval_ms (interval_ms) is an integer > 0 when timeout_ms > 0, and
return {:error, {:invalid_option, reason}} (or similar tuple used in this
module) for invalid values instead of entering the loop; keep existing defaults
( `@default_wait_timeout_ms` and `@default_poll_interval_ms` ) when keys are absent
and only validate after defaults are applied, and reference the existing symbols
timeout_ms, interval_ms, poll_balance, and rpc_url when updating the function.
In `@roadmap/data.json`:
- Line 170: Record for "task 6" is marked "status": "done" but is missing the
done_at timestamp and still has phases.3.status set to "pending"; update the
task entry by adding a proper ISO 8601 done_at datetime and set phases.3.status
to "done" (or the appropriate completed status) so the task-level and
phase-level rollups are consistent—look for the task object labelled "task 6",
the "status" field, the "done_at" field, and the "phases.3.status" property to
apply the updates.
In `@roadmap/tasks.toml`:
- Line 135: The roadmap task metadata only sets status = "done" but lacks the
completion timestamp and a matching phase status; update the same task block to
add a done_at field with an ISO 8601 timestamp (e.g. 2026-05-15T12:00:00Z) and
set phases.3.status (the third phase entry for this task) to "done" so the phase
state aligns with the task status (refer to the task block containing status =
"done" and the phases array).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 63519758-fb96-42c2-8bb8-9addb1e5fd39
📒 Files selected for processing (6)
CHANGELOG.mdROADMAP.mdlib/onchain/tempo/faucet.exroadmap/data.jsonroadmap/tasks.tomltest/onchain/tempo/faucet_test.exs
| "phase": 3, | ||
| "bundle": "faucet_polish", | ||
| "status": "pending", | ||
| "status": "done", |
There was a problem hiding this comment.
Backfill completion metadata when switching task 6 to done.
Task 6 is now done, but this record still lacks done_at, and phases.3.status is still pending. That leaves roadmap rollups internally inconsistent in this file.
Suggested patch
@@
"3": {
"name": "Future Work",
"order": 3,
- "status": "pending"
+ "status": "done"
}
@@
"id": 6,
"phase": 3,
"bundle": "faucet_polish",
"status": "done",
+ "done_at": "2026-05-15",
"title": "Replace Faucet fixed-sleep settle with poll loop",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "status": "done", | |
| "3": { | |
| "name": "Future Work", | |
| "order": 3, | |
| "status": "done" | |
| } | |
| "id": 6, | |
| "phase": 3, | |
| "bundle": "faucet_polish", | |
| "status": "done", | |
| "done_at": "2026-05-15", | |
| "title": "Replace Faucet fixed-sleep settle with poll loop", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@roadmap/data.json` at line 170, Record for "task 6" is marked "status":
"done" but is missing the done_at timestamp and still has phases.3.status set to
"pending"; update the task entry by adding a proper ISO 8601 done_at datetime
and set phases.3.status to "done" (or the appropriate completed status) so the
task-level and phase-level rollups are consistent—look for the task object
labelled "task 6", the "status" field, the "done_at" field, and the
"phases.3.status" property to apply the updates.
| phase = 3 | ||
| bundle = "faucet_polish" | ||
| status = "pending" | ||
| status = "done" |
There was a problem hiding this comment.
Keep TOML roadmap metadata consistent with the new done status.
After marking task 6 as done, this block should also include done_at, and phases.3.status should be updated to match the now-completed phase state.
Suggested patch
@@
[phases.3]
name = "Future Work"
order = 3
-status = "pending"
+status = "done"
@@
[[task]]
id = 6
phase = 3
bundle = "faucet_polish"
status = "done"
+done_at = "2026-05-15"
title = "Replace Faucet fixed-sleep settle with poll loop"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@roadmap/tasks.toml` at line 135, The roadmap task metadata only sets status =
"done" but lacks the completion timestamp and a matching phase status; update
the same task block to add a done_at field with an ISO 8601 timestamp (e.g.
2026-05-15T12:00:00Z) and set phases.3.status (the third phase entry for this
task) to "done" so the phase state aligns with the task status (refer to the
task block containing status = "done" and the phases array).
7 tasks
e-fu
force-pushed
the
task-6-faucet-poll-loop
branch
from
abcf1c7 to
550c341
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/onchain/tempo/faucet_test.exs (1)
203-207: ⚡ Quick winAssert the “no RPC” contract explicitly.
Line 203’s test name claims fail-fast with no RPC, but it currently only checks the returned error text. Add a stub +
refute_receivedso the behavior is truly enforced.Proposed test tightening
test "rejects negative :settle_ms with actionable error (fails fast, no RPC)" do - assert {:error, msg} = Faucet.fresh_funded_wallet(settle_ms: -1) + test_pid = self() + + Req.Test.stub(:should_not_call, fn conn -> + send(test_pid, :rpc_called) + Req.Test.json(conn, %{"jsonrpc" => "2.0", "id" => 1, "result" => ["0xfund"]}) + end) + + assert {:error, msg} = + Faucet.fresh_funded_wallet( + rpc_url: "http://localhost", + req_options: [plug: {Req.Test, :should_not_call}], + settle_ms: -1 + ) + + refute_received :rpc_called assert msg =~ ":settle_ms must be a non-negative integer" assert msg =~ "-1" end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/onchain/tempo/faucet_test.exs` around lines 203 - 207, The test should also assert that no RPC is invoked when Faucet.fresh_funded_wallet(settle_ms: -1) fails fast: add a test-local stub/mock for the RPC client used by Faucet (the module Faucet calls for external RPCs) and set its expectation to zero calls, then after calling Faucet.fresh_funded_wallet assert refute_received (or verify the mock had 0 invocations) to ensure no RPC messages were sent; keep the existing assertions on the error message intact and reference Faucet.fresh_funded_wallet in the updated test name/description.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/onchain/tempo/faucet_test.exs`:
- Around line 203-207: The test should also assert that no RPC is invoked when
Faucet.fresh_funded_wallet(settle_ms: -1) fails fast: add a test-local stub/mock
for the RPC client used by Faucet (the module Faucet calls for external RPCs)
and set its expectation to zero calls, then after calling
Faucet.fresh_funded_wallet assert refute_received (or verify the mock had 0
invocations) to ensure no RPC messages were sent; keep the existing assertions
on the error message intact and reference Faucet.fresh_funded_wallet in the
updated test name/description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68ceed3b-eb31-4aae-96fe-5b76bc5e597d
📒 Files selected for processing (2)
lib/onchain/tempo/faucet.extest/onchain/tempo/faucet_test.exs
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/onchain/tempo/faucet.ex
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the fixed 2.5 s
Process.sleepinOnchain.Tempo.Faucet.fresh_funded_wallet/1with a poll loop oneth_getBalance. The helper now returns as soon as the funding transaction lands on-chain — ~one block (~500 ms on Moderato) instead of a flat ~2.5 s.Closes Task 6 (
[D:3/B:3/U:3 → Eff:1.0]).Option semantics
:settle_msis now the poll timeout (default10_000ms) — previously the sleep duration (2_500ms).settle_ms: 0still skips the wait entirely (unit tests rely on this).:poll_interval_ms(default200ms) tunes the poll cadence.No call-site changes required — existing unit tests pass
settle_ms: 0and integration tests pass nothing (using the new default).Test plan
mix test.json --quiet test/onchain/tempo/faucet_test.exs— all 10 tests pass, including 2 new ones (poll-success, poll-timeout)mix format --check-formatted— cleanmix credo --strict— 0 issuesmix dialyzer.json— 0 warningsmix doctor— 100% doc/spec coverage, validation passedTEMPO_RPC_URL=https://rpc.moderato.tempo.xyz mix test.json --include integrationshould drop wall-clock noticeably (was ≥7.5 s in setup sleeps; expect ~1.5–3 s now, dominated by 3× actual block confirmation)Summary by CodeRabbit
New Features
:settle_ms,:poll_interval_ms).Chores
Tests