-
Notifications
You must be signed in to change notification settings - Fork 3
Add fallback for tx hash resolution by cid in claim token API #326
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a fallback in the native faucet claim flow: after submitting the signed message, the server attempts to fetch an Ethereum tx-hash via Filecoin.EthGetTransactionHashByCid and, on RPC error, logs a warning and returns the CID string. Documentation updated to clarify tx-hash vs CID behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant S as Faucet Server (handle_native_claim)
participant F as Filecoin RPC
C->>S: Submit signed native claim (CID)
S->>F: EthGetTransactionHashByCid(CID)
alt RPC returns tx-hash
F-->>S: tx-hash
note right of S #DFF2E1: Use tx-hash as result string
else RPC error or no hash
F--x S: error
note right of S #FFF4E6: Log warning and fallback to CID string
end
S-->>C: 200 OK with result string (tx-hash or CID)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 #326 +/- ##
==========================================
- Coverage 39.37% 39.31% -0.07%
==========================================
Files 40 40
Lines 2537 2541 +4
==========================================
Hits 999 999
- Misses 1538 1542 +4 ☔ 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: 1
📜 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 (1)
src/utils/lotus_json/signed_message.rs (1)
cid
(36-42)
🔇 Additional comments (2)
src/faucet/server_api.rs (1)
292-296
: Verify test coverage for the fallback path.The codecov report indicates 0% coverage for these lines. The error fallback logic should be tested to ensure it behaves correctly when
eth_get_transaction_hash_by_cid
fails.Consider adding a test case that:
- Mocks
eth_get_transaction_hash_by_cid
to return an error- Verifies that the function returns the CID string
- Confirms that appropriate logging occurs (if implemented per the previous comment)
docs/api-documentation.md (1)
19-21
: Documentation accurately reflects the implementation.The added documentation clearly describes the fallback behavior when transaction hash resolution fails, helping users understand what type of identifier they might receive.
Summary of changes
Changes introduced in this pull request:
This PR fixes the issue reported in https://filecoinproject.slack.com/archives/C08UEF3HNLQ/p1760080099927249
Filecoin.EthGetTransactionHashByCid
, we now return CID instead ofNull
.Preview URL: https://7c29166.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
Bug Fixes
Documentation