-
Notifications
You must be signed in to change notification settings - Fork 2
Handle low faucet balance in claim token API #327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds a docs section "Faucet Top-Up Requests" and inserts an SSR-gated faucet-balance pre-check into Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as Faucet Server (claim_token)
participant R as RPC (wallet_balance)
participant F as Existing Faucet Logic
C->>S: POST /claim_token
S->>R: wallet_balance(faucet_address)
R-->>S: balance
alt balance < required_amount
S-->>C: 500 / ServerError "Faucet is empty, Request top-up"
else balance >= required_amount
S->>F: continue existing claim flow
F-->>S: result
S-->>C: response (unchanged)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #327 +/- ##
==========================================
- Coverage 39.37% 39.09% -0.28%
==========================================
Files 40 40
Lines 2537 2555 +18
==========================================
Hits 999 999
- Misses 1538 1556 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/api-documentation.md(1 hunks)src/faucet/server_api.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/faucet/server_api.rs (2)
src/utils/rpc_context.rs (2)
new(42-59)new(111-113)src/faucet/controller.rs (1)
new(24-85)
🪛 markdownlint-cli2 (0.18.1)
docs/api-documentation.md
157-157: Link text should be descriptive
(MD059, descriptive-link-text)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/faucet/server_api.rs (2)
222-222: Balance check runs before rate limiting (duplicate concern).The
ensure_faucet_has_fundscheck runs here, but rate limiting occurs later insigned_fil_transfer(line 112) andsigned_erc20_transfer(line 158). This creates a TOCTOU race window where concurrent requests can pass the balance check but then be serialized by rate limiting, as already noted in previous reviews.
257-274: Missing native FIL balance check for ERC-20 transfers (duplicate concern).For
CalibnetUSDFC(ERC-20 faucet), line 264 checks the ERC-20 token balance, but ERC-20 transfers require native FIL to pay gas fees. The faucet could have adequate ERC-20 tokens but insufficient native FIL for gas, leading to transaction failures after passing this check, as already noted in previous reviews.
🧹 Nitpick comments (1)
src/faucet/server_api.rs (1)
257-274: Add test coverage for the new balance validation logic.Codecov reports 0% patch coverage for this file. Consider adding unit tests for
ensure_faucet_has_fundsto verify:
- Sufficient balance allows claims to proceed
- Insufficient balance returns the correct error
- Gas estimation is calculated correctly
- Edge cases (balance exactly at threshold, zero balance, etc.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/faucet/server_api.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/faucet/server_api.rs (2)
src/utils/rpc_context.rs (2)
new(42-59)new(111-113)src/faucet/controller.rs (1)
new(24-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e
- GitHub Check: codedov
- GitHub Check: deploy
Summary of changes
Changes introduced in this pull request:
Preview URL: https://1fac3c2.forest-explorer-preview.workers.dev
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
adheres to the team's
documentation standards,
(if possible),
should be reflected in this document.
Summary by CodeRabbit
New Features
Documentation