Restrict addresses on Calibnet USDFC faucet #344
Conversation
sudo-shashank
changed the title
WalkthroughParser and server-side validation for claim_token now accept FaucetInfo and enforce feature-gated restrictions for CalibnetUSDFC (rejecting t0 and specific 0xff…ID forms). Error messages changed to "Invalid address - failed to parse: …". E2E tests and docs updated; 0x-prefixed address parsing behavior refined. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API as claim_token API
participant Parser as parse_and_validate_address(faucet_info)
participant Checker as check_valid_address (feature-gated)
User->>API: POST /claim_token { address }
API->>Parser: parse_and_validate_address(address, faucet_info)
Parser->>Checker: check_valid_address(address, faucet_info)
alt Restricted (CalibnetUSDFC + t0 or 0xff...ID)
Checker-->>Parser: Err (BAD_REQUEST)
Parser-->>API: ServerFnError 400
API-->>User: 400 Bad Request ("Invalid address - failed to parse: ...")
else Not restricted
Checker-->>Parser: Ok
Parser->>Parser: parse using faucet_info.network()
alt Parsed successfully
Parser-->>API: ParsedAddress
API-->>User: proceed with claim
else Parse failure
Parser-->>API: ServerFnError 400 ("Invalid address - failed to parse: ...")
API-->>User: 400 Bad Request
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-09-30T08:00:02.860ZApplied to files:
📚 Learning: 2025-09-29T08:51:56.994ZApplied to files:
🧬 Code graph analysis (1)src/faucet/server_api.rs (3)
⏰ 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). (6)
🔇 Additional comments (5)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #344 +/- ##
==========================================
+ Coverage 39.11% 41.09% +1.98%
==========================================
Files 40 40
Lines 2595 2667 +72
==========================================
+ Hits 1015 1096 +81
+ Misses 1580 1571 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2e/test_claim_token_api_config.js (1)
299-314: Add uppercase coverage for restricted addressesPlease add a scenario using an uppercase variant (e.g.,
0xFF0000…orT0…). Without it, the server-side restriction can be bypassed by sending uppercase input, so the test suite will miss regressions even after the fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/api-documentation.md(1 hunks)e2e/test_claim_token_api_config.js(1 hunks)src/faucet/server_api.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/faucet/server_api.rs (1)
src/utils/address.rs (1)
parse_address(49-76)
⏰ 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). (5)
- GitHub Check: codedov
- GitHub Check: E2E API Tests
- GitHub Check: E2E Browser Tests
- GitHub Check: E2E Claim Token API CORS Tests
- GitHub Check: deploy
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/faucet/server_api.rs (1)
286-290: Minor grammar improvement and test coverage concern.The error message should use "Ethereum-style" (hyphenated) as a compound adjective. Additionally, the codecov report shows 0% coverage for these new lines.
Apply this diff for the grammar fix:
- return Err(ServerFnError::ServerError("Use of ID addresses or their corresponding Ethereum style 0xff...ID addresses is restricted when claiming tokens from the CalibnetUSDFC faucet.".to_string())); + return Err(ServerFnError::ServerError("Use of ID addresses or their corresponding Ethereum-style 0xff...ID addresses is restricted when claiming tokens from the CalibnetUSDFC faucet.".to_string()));Regarding test coverage: please ensure the restriction logic is covered by tests. The e2e tests in
test_claim_token_api_config.jsshould exercise both thet0and0xffaddress rejection paths to confirm the 400 BAD_REQUEST response.
📜 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 (1)
src/utils/address.rs (1)
parse_address(49-76)
⏰ 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). (6)
- GitHub Check: deploy
- GitHub Check: E2E Browser Tests
- GitHub Check: E2E Claim Token API CORS Tests
- GitHub Check: E2E API Tests
- GitHub Check: codedov
- GitHub Check: lint
🔇 Additional comments (2)
src/faucet/server_api.rs (2)
274-279: LGTM! Case-sensitivity bypass fixed.The normalization with
trim().to_lowercase()correctly addresses the previous concern about uppercase inputs bypassing the restriction. The prefix checks will catch all ID addresses in both0xff(Ethereum-style) andt0(Filecoin-style) formats.
225-225: LGTM! Correct parameter update.The call site correctly updated to pass
faucet_infoinstead ofnetwork, matching the new signature ofparse_and_validate_address.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/test_claim_token_api_config.js (1)
286-301: Test logic is correct, but consider organizational improvements.The core test logic correctly validates the new address restriction behavior for CalibnetUSDFC:
- Expects immediate rejection (BAD_REQUEST/400) for restricted addresses
- No wait time or wallet cap error expected
- Uses the correct restricted address formats (0xff...ID and t0)
However, consider these optional improvements:
Test Organization: These tests are in
RATE_LIMIT_TEST_WALLET_CAP_CASESbut test address restrictions rather than wallet caps. Consider creating a separate test category likeRESTRICTED_ADDRESS_CASESor moving these to a more semantically appropriate location for better maintainability.Error Message Validation: Unlike the
INVALID_REQUESTStests (lines 83-142), these tests don't include anexpectedErrorContainsfield. Adding error message validation would help ensure users receive informative feedback:expectedErrorContains: 'restricted' // or appropriate error messageNaming: The test names contain "restricted address (RESTRICTED)" which is redundant. Consider simplifying to just "restricted address" in the name or removing the suffix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e/test_claim_token_api_config.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: sudo-shashank
Repo: ChainSafe/forest-explorer PR: 315
File: src/faucet/server_api.rs:308-311
Timestamp: 2025-09-30T08:00:02.860Z
Learning: In the forest-explorer project (src/faucet/server_api.rs), using `rpc.mpool_get_nonce(from)` for ERC-20 transactions works correctly even though it queries the Filecoin mempool nonce rather than using `eth_getTransactionCount`. This has been tested and confirmed to work by the maintainers.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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
🧠 Learnings (1)
📓 Common learnings
Learnt from: sudo-shashank
Repo: ChainSafe/forest-explorer PR: 315
File: src/faucet/server_api.rs:308-311
Timestamp: 2025-09-30T08:00:02.860Z
Learning: In the forest-explorer project (src/faucet/server_api.rs), using `rpc.mpool_get_nonce(from)` for ERC-20 transactions works correctly even though it queries the Filecoin mempool nonce rather than using `eth_getTransactionCount`. This has been tested and confirmed to work by the maintainers.
🧬 Code graph analysis (1)
src/faucet/server_api.rs (1)
src/utils/address.rs (1)
parse_address(49-76)
⏰ 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). (4)
- GitHub Check: E2E API Tests
- GitHub Check: E2E Claim Token API CORS Tests
- GitHub Check: E2E Browser Tests
- GitHub Check: deploy
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/faucet/server_api.rs (2)
274-289: Consider refactoring the validation logic for clarityThe validation logic works correctly but has some issues:
Non-idiomatic side effect: The condition
address.into_eth_address().is_err()calls a consuming method within a boolean expression, relying on short-circuit evaluation to avoid movingaddresswhen not needed. While this works (assumingAddressisCopy), it's harder to understand.Redundant conversion: For CalibnetUSDFC,
into_eth_address()may be called twice—once here in validation and again inhandle_erc20_claim(line 386). SinceAddressis likelyCopy, this isn't a correctness issue, but it is inefficient.Silent status-code failure: Line 284-285 uses
use_context::<ResponseOptions>().map(...), which silently does nothing if the context is missing. While defensive, consider usingexpect_context(as done inset_response_statusat line 426) to ensure the status code is always set.Consider this refactor for improved clarity:
fn check_valid_address(address: Address, faucet_info: FaucetInfo) -> Result<(), ServerFnError> { use crate::utils::address::AddressAlloyExt; use fvm_shared::address::Protocol; if matches!(faucet_info, FaucetInfo::CalibnetUSDFC) - && (address.protocol() == Protocol::ID || address.into_eth_address().is_err()) + && !is_valid_erc20_address(address) { log::error!("Invalid address: {:?}", address); - leptos::context::use_context::<ResponseOptions>() - .map(|_| set_response_status(StatusCode::BAD_REQUEST)); + set_response_status(StatusCode::BAD_REQUEST); return Err(ServerFnError::ServerError("Use of ID addresses or their corresponding Ethereum style 0xff...ID addresses is restricted when claiming tokens from the CalibnetUSDFC faucet.".to_string())); } Ok(()) } + +fn is_valid_erc20_address(address: Address) -> bool { + // Only Delegated addresses (f4/t4) can receive ERC-20 tokens + // ID addresses and f1/f2/f3 addresses cannot + address.protocol() == Protocol::Delegated + && address.into_eth_address().is_ok() +}This makes the intent explicit: we're checking if it's a valid ERC-20 address rather than an invalid one.
429-635: Comprehensive test coverage with minor cfg issueThe test suite excellently covers all address types across different faucet configurations, properly validating that:
- CalibnetUSDFC restricts ID addresses (t0, 0xff...ID) and non-EVM-compatible addresses (t1, t2, t3)
- CalibnetUSDFC allows delegated addresses (t4) and regular Ethereum addresses (0x...)
- Other faucets (MainnetFIL, CalibnetFIL) allow all address types
However, the test module configuration could be improved:
Apply this diff to ensure tests are only compiled during test builds:
-#[cfg(feature = "ssr")] +#[cfg(all(test, feature = "ssr"))] mod tests {This prevents the test module from being included in production builds while still requiring the "ssr" feature for server-side-specific test logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/faucet/server_api.rs(3 hunks)src/utils/address.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/faucet/server_api.rs (1)
src/utils/address.rs (1)
parse_address(49-76)
⏰ 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). (6)
- GitHub Check: lint
- GitHub Check: E2E Browser Tests
- GitHub Check: E2E API Tests
- GitHub Check: E2E Claim Token API CORS Tests
- GitHub Check: codedov
- GitHub Check: deploy
🔇 Additional comments (3)
src/utils/address.rs (1)
66-72: LGTM - Prefix corrected to match Filecoin ID address formatThe updated prefix
"0xff0000000000000000000000"(12 bytes: 0xff + 11 zero bytes) correctly matches theMASKED_ID_PREFIXconstant and aligns with the Filecoin EVM address specification for ID-to-Ethereum mappings. This ensures that any 0x-prefixed address beginning with this pattern is properly parsed as an ID address, while other 0x addresses are correctly treated as delegated addresses.src/faucet/server_api.rs (2)
225-225: LGTM - Function call updated to support faucet-aware validationThe change from passing
networktofaucet_infoenables the validation logic to enforce faucet-specific address restrictions, which is essential for this PR's objective.
292-310: LGTM - Clean integration of validation logicThe refactored function properly integrates the new
check_valid_addressvalidation while maintaining clear error handling and logging. The signature change to acceptfaucet_infoenables faucet-specific restrictions as intended.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/api-documentation.md(3 hunks)e2e/test_claim_token_api_config.js(2 hunks)src/faucet/server_api.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api-documentation.md
- e2e/test_claim_token_api_config.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/faucet/server_api.rs (2)
src/utils/address.rs (1)
parse_address(49-76)src/faucet/constants.rs (1)
network(152-157)
🔇 Additional comments (3)
src/faucet/server_api.rs (3)
225-225: LGTM!The updated call correctly passes
faucet_infoto enable address validation logic.
293-311: LGTM!The refactoring cleanly integrates the new validation logic, and the error handling flows are consistent.
430-510: LGTM!The unit tests are comprehensive and correctly validate the address restriction logic across all faucet types and address formats.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/faucet/server_api.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/faucet/server_api.rs (2)
src/utils/address.rs (1)
parse_address(49-76)src/faucet/constants.rs (1)
network(152-157)
⏰ 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). (6)
- GitHub Check: E2E Claim Token API CORS Tests
- GitHub Check: E2E Browser Tests
- GitHub Check: E2E API Tests
- GitHub Check: codedov
- GitHub Check: lint
- GitHub Check: deploy
🔇 Additional comments (3)
src/faucet/server_api.rs (3)
225-225: LGTM! Signature change enables faucet-specific validation.Passing
faucet_infoinstead of justnetworkallows the validation logic to apply different rules based on the faucet type, which is exactly what's needed for CalibnetUSDFC restrictions.
274-290: LGTM! Address validation logic is correct.The validation correctly rejects ID addresses (including
0xff...IDforms that are parsed asProtocol::ID) and non-Ethereum-compatible addresses (t1, t2, t3) for CalibnetUSDFC, while allowing delegated t4 and native 0x Ethereum addresses. The defensive context check on line 284 prevents panics when the ResponseOptions context is unavailable.
430-510: Excellent test coverage!The tests comprehensively cover all three faucet types with appropriate address format validation. The test design is good:
- MainnetFIL and CalibnetFIL correctly allow all address types
- CalibnetUSDFC correctly allows only Ethereum-compatible addresses (t4, 0x) and rejects ID-mapped and native Filecoin addresses (t0, t1, t2, t3, 0xff...ID)
The
assert_address!macro efficiently tests bothcheck_valid_addressandparse_and_validate_address, ensuring consistent behavior between the two functions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/faucet/server_api.rs (1)
430-430: Remove unnecessary attribute.The macro is actively used throughout the test functions (lines 462, 478, 497, 501-506), so the
#[allow(unused_macros)]attribute is unnecessary and misleading.Apply this diff:
- #[allow(unused_macros)] macro_rules! assert_address {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/faucet/server_api.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/faucet/server_api.rs (2)
src/utils/address.rs (1)
parse_address(49-76)src/faucet/constants.rs (1)
network(152-157)
⏰ 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). (6)
- GitHub Check: lint
- GitHub Check: E2E API Tests
- GitHub Check: E2E Claim Token API CORS Tests
- GitHub Check: deploy
- GitHub Check: E2E Browser Tests
- GitHub Check: codedov
🔇 Additional comments (5)
src/faucet/server_api.rs (5)
225-225: LGTM! Call site correctly updated.The change from passing
networktofaucet_infoaligns with the updated function signature and enables the new restriction checks.
274-288: LGTM! Validation logic correctly implements restrictions.The function properly restricts CalibnetUSDFC to Ethereum-compatible addresses by rejecting ID addresses (including 0xff...ID format, which
parse_addressconverts to ID protocol) and other non-Ethereum-compatible types (t1, t2, t3). The error message is clear and user-friendly.
290-309: LGTM! Parse-and-validate flow is well-structured.The function correctly parses the address and applies the new validation checks. Error handling is appropriate with clear messages and proper status codes. The defensive check concern from past reviews is addressed by the updated
set_response_statusimplementation.
425-427: LGTM! Defensive implementation prevents panics.The change from
expect_contexttouse_contextwithmapensures the function won't panic when ResponseOptions context is unavailable (e.g., in test environments). This addresses the concern raised in past reviews.
450-507: LGTM! Comprehensive test coverage.The test suite thoroughly validates the restriction logic across all faucet types with multiple address formats. The CalibnetUSDFC tests specifically verify that only Ethereum-compatible addresses (t4 delegated and native 0x) are accepted while ID, secp256k1, actor, BLS, and 0xff...ID formats are properly rejected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/faucet/server_api.rs (1)
274-288: Address validation logic is correct and comprehensive.The dual-check approach correctly restricts all inaccessible address types for CalibnetUSDFC:
Protocol::IDcheck blocks both t0 and 0xff...ID addresses (which parse as ID)into_eth_address().is_err()check blocks t1, t2, and t3 addressesThis allows only the intended address types: t4 (delegated) and native 0x addresses.
Optional: Consider adding a brief comment explaining the business reason.
While the error message is clear, a brief inline comment explaining why these addresses are restricted (tokens sent to them are not accessible/recoverable) would help future maintainers understand the business context.
#[cfg(feature = "ssr")] fn check_valid_address(address: Address, faucet_info: FaucetInfo) -> Result<(), ServerFnError> { use crate::utils::address::AddressAlloyExt; use fvm_shared::address::Protocol; + // For CalibnetUSDFC, only allow Ethereum-compatible addresses (t4/0x) because + // tokens sent to ID/secp256k1/actor/BLS addresses are not accessible/recoverable if matches!(faucet_info, FaucetInfo::CalibnetUSDFC) && (address.protocol() == Protocol::ID || address.into_eth_address().is_err())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/faucet/server_api.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/faucet/server_api.rs (3)
src/utils/address.rs (1)
parse_address(49-76)src/utils/rpc_context.rs (3)
res(101-101)use_context(65-67)use_context(66-66)src/faucet/constants.rs (1)
network(152-157)
⏰ 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). (6)
- GitHub Check: E2E API Tests
- GitHub Check: E2E Claim Token API CORS Tests
- GitHub Check: E2E Browser Tests
- GitHub Check: lint
- GitHub Check: deploy
- GitHub Check: codedov
🔇 Additional comments (3)
src/faucet/server_api.rs (3)
290-309: LGTM! Validation flow is well-structured.The updated function correctly:
- Parses the address using the network from
faucet_info- Validates the parsed address with
check_valid_address- Sets appropriate HTTP status codes for both parse and validation failures
- Provides clear error messages distinguishing parse errors from validation errors
423-428: LGTM! Safe context handling.The explicit
if let Somecheck safely handles scenarios where the Leptos context is unavailable (such as in certain test environments), making the function robust across different execution contexts.
430-508: Excellent test coverage!The test suite comprehensively validates the address restriction logic:
- Tests all faucet types (MainnetFIL, CalibnetFIL, CalibnetUSDFC)
- Covers all address protocols (ID, secp256k1, actor, BLS, delegated, Ethereum)
- Validates both positive and negative cases for CalibnetUSDFC restrictions
- Uses a clever macro to reduce duplication while maintaining clarity
The tests confirm that only t4 (delegated) and native 0x addresses are accepted for CalibnetUSDFC, while all other faucets accept all address types.
Summary of changes
Changes introduced in this pull request:
tUSDFCtokens claimed for ID address or it's corresponding eth style0xff...IDaddress are not accessible so this PR restricts these addresses onCalibnetUSDFCfaucet.Preview URL: https://1432c3b.forest-explorer-preview.workers.dev
Reference issue to close (if applicable)
Closes #335
Other information and links
Change checklist
adheres to the team's
documentation standards,
(if possible),
should be reflected in this document.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests