Added support for f4 addresses in forest-wallet#6710
Merged
sudo-shashank merged 29 commits intomainfrom Mar 12, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds support for f4 (delegated / Ethereum-style) addresses across wallet send, message-pool validation, and RPC helpers; updates wallet tests/scripts and changelog. Key changes: ETH-target resolution, delegated-key EIP-1559 signing, delegated recipient EthAddress validation, and making Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant WalletCmd as Wallet Send<br><span style="font-size:80%">(wallet_cmd.rs)</span>
participant RPC as RPC Methods<br><span style="font-size:80%">(eth.rs)</span>
participant State as Chain State
participant MsgPool as Message Pool<br><span style="font-size:80%">(msg_pool.rs)</span>
participant Network as Blockchain
User->>WalletCmd: send(target, amount)
WalletCmd->>RPC: is_eth_address(target)
RPC-->>WalletCmd: true / false
alt target is ETH-style
WalletCmd->>WalletCmd: convert ETH -> f4 Filecoin address
WalletCmd->>State: resolve f4 -> ID (if needed)
State-->>WalletCmd: ID address or error
WalletCmd->>WalletCmd: construct EIP-1559 tx, sign with Delegated key
else standard FIL send
WalletCmd->>WalletCmd: construct standard message, sign normally
end
WalletCmd->>MsgPool: submit message
MsgPool->>MsgPool: validate recipient (ensure valid EthAddress if Delegated)
MsgPool->>Network: propagate message
Network-->>User: confirmation/receipt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
sudo-shashank
requested review from
LesnyRumcajs and
hanabi1224
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
CHANGELOG.md (1)
34-34: Use issue number instead of PR number in changelog entry.Per project guidelines, when both an issue and a PR exist, the changelog should reference the issue number. Since this PR closes issue
#5766, the entry should link to the issue.Suggested fix
-- [`#6710`](https://github.com/ChainSafe/forest/pull/6710): Added support for f4 addresses in forest-wallet. +- [`#5766`](https://github.com/ChainSafe/forest/issues/5766): Added support for f4 addresses in forest-wallet.Based on learnings: "In CHANGELOG.md entries, when both an issue and a PR exist for a change, reference the issue number in the entry using the format
#ISSUE_NO: . Use PR numbers only if there is no corresponding issue."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 34, Replace the PR reference in the CHANGELOG.md entry that currently reads "[`#6710`](https://github.com/ChainSafe/forest/pull/6710): Added support for f4 addresses in forest-wallet." with the corresponding issue reference "[`#5766`](https://github.com/ChainSafe/forest/issues/5766): Added support for f4 addresses in forest-wallet." — locate the line containing "Added support for f4 addresses in forest-wallet" and swap the PR link/number for the issue link/number to follow the project changelog convention.src/rpc/methods/eth.rs (1)
1015-1022: Add documentation for the newly public function.The function is now exposed publicly but lacks a doc comment. Per coding guidelines, all public functions should be documented. Consider adding a brief description of what constitutes an "ETH address" in the Filecoin context (i.e., a delegated address with a valid
DelegatedAddresspayload).📝 Suggested documentation
+/// Returns `true` if the given address is a delegated (f4) address that can be +/// represented as an Ethereum address. pub fn is_eth_address(addr: &VmAddress) -> bool { if addr.protocol() != Protocol::Delegated { return false; } let f4_addr: Result<DelegatedAddress, _> = addr.payload().try_into(); f4_addr.is_ok() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/rpc/methods/eth.rs` around lines 1015 - 1022, Add a doc comment above the public function is_eth_address explaining that an "ETH address" in this codebase is a Filecoin delegated address (Protocol::Delegated) whose payload successfully parses into a DelegatedAddress; reference VmAddress, Protocol::Delegated, payload(), try_into(), and DelegatedAddress so readers understand the checks performed by is_eth_address. Keep the doc short (one or two sentences) describing the predicate semantics and return value (true when the address is delegated and its payload is a valid DelegatedAddress).src/wallet/subcommands/wallet_cmd.rs (1)
571-576: Add context to chain ID parsing for clearer error messages.The chain ID parsing could fail if the RPC returns an unexpected format. Adding context would help diagnose issues.
📝 Suggested improvement
let eth_chain_id = u64::from_str_radix( EthChainId::call(&backend.remote, ()) .await? .trim_start_matches("0x"), 16, - )?; + ) + .context("failed to parse eth_chainId as hex")?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/subcommands/wallet_cmd.rs` around lines 571 - 576, The parsing of the RPC chain ID into eth_chain_id can fail without context; wrap the result of EthChainId::call(&backend.remote, ()).await and the u64::from_str_radix(...) parse with a contextual error (e.g., using anyhow::Context or map_err) that includes the raw RPC response and a short message like "parsing chain id from RPC response" so failures show the returned string and clarify whether trimming/hex parsing failed; update the code around EthChainId::call and the eth_chain_id assignment in wallet_cmd.rs to add that context.scripts/tests/calibnet_wallet_check.sh (1)
159-171: Consider adding error handling for RPC responses.The RPC calls extract
.resultdirectly without checking if the call succeeded. If the RPC returns an error (e.g., address not found in state),$ETH_ADDR_TWOwill be"null"and the subsequentsendcommand will fail with an unclear error.🛡️ Suggested improvement
ETH_ADDR_TWO=$(curl -s -X POST "$FOREST_URL" \ -H 'Content-Type: application/json' \ -H "Authorization: Bearer $ADMIN_TOKEN" \ --data "$(jq -n --arg addr "$ADDR_TWO" '{jsonrpc: "2.0", id: 1, method: "Filecoin.FilecoinAddressToEthAddress", params: [$addr, "pending"]}')" \ | jq -r '.result') +if [[ "$ETH_ADDR_TWO" == "null" || -z "$ETH_ADDR_TWO" ]]; then + echo "Failed to resolve ETH address for $ADDR_TWO" + exit 1 +fi echo "ETH address: $ETH_ADDR_TWO"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/tests/calibnet_wallet_check.sh` around lines 159 - 171, The RPC responses for the Filecoin.FilecoinAddressToEthAddress calls are parsed directly into ETH_ADDR_TWO and ETH_ADDR_THREE without checking for errors or null results; update the curl/jq logic that assigns ETH_ADDR_TWO and ETH_ADDR_THREE to first capture the full JSON response, check for a non-null .result and absence of .error, and if the response contains .error or .result is null/empty, print a clear error (including the JSON error) and exit non‑zero before proceeding to any send operations; reference the existing variables/assignments ETH_ADDR_TWO, ETH_ADDR_THREE and the Filecoin.FilecoinAddressToEthAddress RPC invocation to locate where to add this validation and early exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/subcommands/wallet_cmd.rs`:
- Around line 523-540: The current conditional uses "is_eth_address(&from) ||
is_0x_recipient" which lets non-delegated senders select EVM methods; change the
logic so only delegated (f4) senders trigger the EVM/EAM branch: replace the
condition with "if is_eth_address(&from) { ... } else { METHOD_SEND }" or
alternatively add an explicit validation that returns an error when
is_0x_recipient && !is_eth_address(&from); update the block around the
StateLookupID::call, EAMMethod::CreateExternal/EVMMethod::InvokeContract
selection to only run when is_eth_address(&from) and ensure this matches the
signing path that expects SignatureType::Delegated (sig_type).
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 34: Replace the PR reference in the CHANGELOG.md entry that currently
reads "[`#6710`](https://github.com/ChainSafe/forest/pull/6710): Added support for
f4 addresses in forest-wallet." with the corresponding issue reference
"[`#5766`](https://github.com/ChainSafe/forest/issues/5766): Added support for f4
addresses in forest-wallet." — locate the line containing "Added support for f4
addresses in forest-wallet" and swap the PR link/number for the issue
link/number to follow the project changelog convention.
In `@scripts/tests/calibnet_wallet_check.sh`:
- Around line 159-171: The RPC responses for the
Filecoin.FilecoinAddressToEthAddress calls are parsed directly into ETH_ADDR_TWO
and ETH_ADDR_THREE without checking for errors or null results; update the
curl/jq logic that assigns ETH_ADDR_TWO and ETH_ADDR_THREE to first capture the
full JSON response, check for a non-null .result and absence of .error, and if
the response contains .error or .result is null/empty, print a clear error
(including the JSON error) and exit non‑zero before proceeding to any send
operations; reference the existing variables/assignments ETH_ADDR_TWO,
ETH_ADDR_THREE and the Filecoin.FilecoinAddressToEthAddress RPC invocation to
locate where to add this validation and early exit.
In `@src/rpc/methods/eth.rs`:
- Around line 1015-1022: Add a doc comment above the public function
is_eth_address explaining that an "ETH address" in this codebase is a Filecoin
delegated address (Protocol::Delegated) whose payload successfully parses into a
DelegatedAddress; reference VmAddress, Protocol::Delegated, payload(),
try_into(), and DelegatedAddress so readers understand the checks performed by
is_eth_address. Keep the doc short (one or two sentences) describing the
predicate semantics and return value (true when the address is delegated and its
payload is a valid DelegatedAddress).
In `@src/wallet/subcommands/wallet_cmd.rs`:
- Around line 571-576: The parsing of the RPC chain ID into eth_chain_id can
fail without context; wrap the result of EthChainId::call(&backend.remote,
()).await and the u64::from_str_radix(...) parse with a contextual error (e.g.,
using anyhow::Context or map_err) that includes the raw RPC response and a short
message like "parsing chain id from RPC response" so failures show the returned
string and clarify whether trimming/hex parsing failed; update the code around
EthChainId::call and the eth_chain_id assignment in wallet_cmd.rs to add that
context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a650bf55-7681-4ba6-9616-5d18028e2c8d
📒 Files selected for processing (5)
CHANGELOG.mdscripts/tests/calibnet_wallet_check.shsrc/message_pool/msgpool/msg_pool.rssrc/rpc/methods/eth.rssrc/wallet/subcommands/wallet_cmd.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/wallet/subcommands/wallet_cmd.rs (3)
552-557: Consider adding context to chain ID parsing error.If the
EthChainIdRPC returns an unexpected format, the genericfrom_str_radixerror may not be immediately clear to users. Adding.context()would improve debuggability.♻️ Suggested improvement
let eth_chain_id = u64::from_str_radix( EthChainId::call(&backend.remote, ()) .await? .trim_start_matches("0x"), 16, -)?; +) +.context("Failed to parse EthChainId as hex")?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/subcommands/wallet_cmd.rs` around lines 552 - 557, The chain ID parsing using u64::from_str_radix on the result of EthChainId::call(&backend.remote, ()) can produce a generic parse error; update the parse call to attach contextual information (e.g., using .context() / .with_context()) so failures include the raw RPC value and a clear message that parsing the hex chain ID failed, referencing EthChainId::call and the eth_chain_id parsing step.
773-779: Clarify test intent forf0tof4with FIL-format input.This test verifies that when a non-delegated sender (f0) sends to a delegated recipient (f4) using FIL address format input (not 0x), the method is
METHOD_SEND. This is intentional based on theis_0x_recipientflag tracking the input format rather than the address protocol.Consider adding a brief comment explaining this distinction, as it may not be immediately obvious to future maintainers:
// When recipient is entered as f410f... (FIL format), not 0x, use METHOD_SEND // for non-delegated senders. EVM methods are only used when: // 1. Sender is delegated (f4), OR // 2. Recipient was entered as 0x address🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/subcommands/wallet_cmd.rs` around lines 773 - 779, The test test_resolve_method_num_send_to_delegated should include a short clarifying comment: explain that resolve_method_num(&from, &to, false) uses the is_0x_recipient flag (false) so a FIL-format recipient "f4..." yields METHOD_SEND for a non-delegated sender; note that EVM methods are only selected when the sender is delegated (f4) or the recipient was entered as a 0x address. Add this comment near the assertion referencing resolve_method_num and METHOD_SEND to make the intent clear to future maintainers.
653-662: Consider simplifying the return type.The function never returns an error, so the
anyhow::Result<u64>return type could be simplified to justu64. This would make the API clearer and remove unnecessary?operators at call sites.♻️ Suggested simplification
-fn resolve_method_num(from: &Address, to: &Address, is_0x_recipient: bool) -> anyhow::Result<u64> { +fn resolve_method_num(from: &Address, to: &Address, is_0x_recipient: bool) -> u64 { if !is_eth_address(from) && !is_0x_recipient { - return Ok(METHOD_SEND); + return METHOD_SEND; } if *to == Address::ETHEREUM_ACCOUNT_MANAGER_ACTOR { - Ok(EAMMethod::CreateExternal as u64) + EAMMethod::CreateExternal as u64 } else { - Ok(EVMMethod::InvokeContract as u64) + EVMMethod::InvokeContract as u64 } }And update the call site at line 521:
-let method_num = resolve_method_num(&from, &to, is_0x_recipient)?; +let method_num = resolve_method_num(&from, &to, is_0x_recipient);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/subcommands/wallet_cmd.rs` around lines 653 - 662, The function resolve_method_num currently returns anyhow::Result<u64> but never errors; change its signature to return u64 (fn resolve_method_num(from: &Address, to: &Address, is_0x_recipient: bool) -> u64) and replace the Ok(...) returns with plain u64 values (e.g., METHOD_SEND, EAMMethod::CreateExternal as u64, EVMMethod::InvokeContract as u64); then update all callers (the sites that call resolve_method_num and currently unwrap with ? or expect a Result) to accept a u64 directly and remove the unnecessary ? operators or Result handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/wallet/subcommands/wallet_cmd.rs`:
- Around line 552-557: The chain ID parsing using u64::from_str_radix on the
result of EthChainId::call(&backend.remote, ()) can produce a generic parse
error; update the parse call to attach contextual information (e.g., using
.context() / .with_context()) so failures include the raw RPC value and a clear
message that parsing the hex chain ID failed, referencing EthChainId::call and
the eth_chain_id parsing step.
- Around line 773-779: The test test_resolve_method_num_send_to_delegated should
include a short clarifying comment: explain that resolve_method_num(&from, &to,
false) uses the is_0x_recipient flag (false) so a FIL-format recipient "f4..."
yields METHOD_SEND for a non-delegated sender; note that EVM methods are only
selected when the sender is delegated (f4) or the recipient was entered as a 0x
address. Add this comment near the assertion referencing resolve_method_num and
METHOD_SEND to make the intent clear to future maintainers.
- Around line 653-662: The function resolve_method_num currently returns
anyhow::Result<u64> but never errors; change its signature to return u64 (fn
resolve_method_num(from: &Address, to: &Address, is_0x_recipient: bool) -> u64)
and replace the Ok(...) returns with plain u64 values (e.g., METHOD_SEND,
EAMMethod::CreateExternal as u64, EVMMethod::InvokeContract as u64); then update
all callers (the sites that call resolve_method_num and currently unwrap with ?
or expect a Result) to accept a u64 directly and remove the unnecessary ?
operators or Result handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 75080a7b-7ac7-4d63-9a06-19fc3f0f1308
📒 Files selected for processing (1)
src/wallet/subcommands/wallet_cmd.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/subcommands/wallet_cmd.rs`:
- Around line 513-519: The current map_err on StateLookupID::call hides the
original RPC error; change the error handling so
StateLookupID::call(&backend.remote, (to.clone(), ApiTipsetKey(None))).await
preserves the original error and adds context with anyhow::Context (e.g., use
.context("...") or .with_context(...) on the awaited Result) rather than
unconditionally mapping every failure to “could not find id address”; if the RPC
client exposes a distinct NotFound/404 error, match that case and produce the
specialized “could not find id address for {to}” message while for all other
errors return the original error with added context so transport/server failures
are not misreported.
- Around line 641-649: The code in resolve_target_address currently returns
false for Filecoin-form targets regardless of whether the resolved address is an
delegated f4/ETH-mapped address; update resolve_target_address (and the similar
logic at the other occurrence around the 653-661 block) to compute the boolean
flag from the resolved Address by calling is_eth_address(&addr) (or equivalent
helper) after converting the input to an Address, rather than hard-coding false
for non-0x input; ensure both branches that produce (addr, bool) use
is_eth_address(&addr) so f4-style addresses like f410f... are treated the same
as 0x... ETH recipients when deciding the EVM method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fe600f72-5401-4804-8680-04775c91fadd
📒 Files selected for processing (1)
src/wallet/subcommands/wallet_cmd.rs
hanabi1224
previously approved these changes
Mar 11, 2026
sudo-shashank
force-pushed
the
shashank/add-f4-support
branch
from
1b7dbe1 to
3866434
Compare
sudo-shashank
force-pushed
the
shashank/add-f4-support
branch
from
f1be6ed to
4fa870a
Compare
LesnyRumcajs
approved these changes
Mar 12, 2026
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
6 tasks
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
3 participants
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 of changes
Changes introduced in this pull request:
forest-walletwith test.Reference issue to close (if applicable)
Closes #5766
Other information and links
Change checklist
Outside contributions
Summary by CodeRabbit