feat(rpc): add v1 RPC method Filecoin.FilecoinAddressToEthAddress
#6323
Conversation
WalkthroughAdded a new RPC method Filecoin.FilecoinAddressToEthAddress (API v1/v2) that converts a Filecoin address to an EthAddress, performing direct conversion or tipset-based ID resolution when needed; registered the method, updated EthAddressToFilecoinAddress metadata, adjusted test harness parsing, and added tests and snapshots. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
hanabi1224
requested review from
LesnyRumcajs and
akaladarshi
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/tool/subcommands/api_cmd/api_compare_tests.rs (1)
1459-1472: New ETH<->Filecoin address tests are wired correctly; consider an extra state‑resolved caseThe switch to
"…".parse().unwrap()forEthAddressToFilecoinAddresskeeps behavior identical and is idiomatic with type inference, and addingFilecoinAddressToEthAddresshere ensures the new RPC is covered by the Lotus/Forest comparison harness. If you want broader coverage, you might optionally add a second case using a non‑delegated (e.g., secp/BLS) Filecoin address that requires ID lookup at a specific block height to exercise the state‑dependent path of the new method.src/rpc/methods/eth.rs (1)
2765-2795: FilecoinAddressToEthAddress two‑stage resolution looks correct and consistentThe new RPC cleanly separates:
- A fast, purely structural path for addresses already encodable as EthAddress (
Protocol::IDand valid EAMDelegated), and- A stateful path for other Filecoin addresses, resolving the ID at a tipset chosen by
blockParam(defaulting toExtPredefined::Finalized) and then converting that ID viaEthAddress::from_filecoin_address.This aligns with existing helpers (
tipset_by_ext_block_number_or_hash,lookup_required_id,EthAddress::from_filecoin_address) and gives callers a way to pin resolution to a specific block while keeping a safe default. I don’t see correctness issues here; the behavior on non-resolvable or invalid addresses will naturally surface as errors from the underlying helpers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)src/rpc/methods/eth.rs(2 hunks)src/rpc/mod.rs(1 hunks)src/tool/subcommands/api_cmd/api_compare_tests.rs(1 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.350Z
Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.
📚 Learning: 2025-09-02T14:23:53.808Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/tool/subcommands/api_cmd/test_snapshots.txt:206-252
Timestamp: 2025-09-02T14:23:53.808Z
Learning: In the Forest project, .rpcsnap.json.zst snapshot files listed in src/tool/subcommands/api_cmd/test_snapshots.txt are not stored in the repository itself but are downloaded from a DigitalOcean (DO) bucket when needed. The manifest file serves as an index/catalog of available snapshots.
Applied to files:
src/tool/subcommands/api_cmd/test_snapshots.txt
📚 Learning: 2025-09-02T10:05:34.350Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.350Z
Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.
Applied to files:
src/tool/subcommands/api_cmd/api_compare_tests.rsCHANGELOG.mdsrc/rpc/mod.rssrc/rpc/methods/eth.rs
🧬 Code graph analysis (1)
src/rpc/methods/eth.rs (2)
src/rpc/reflect/mod.rs (1)
all_with_v2(156-158)src/rpc/methods/eth/types.rs (1)
from_filecoin_address(156-182)
⏰ 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). (7)
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
- GitHub Check: All lint checks
🔇 Additional comments (4)
src/tool/subcommands/api_cmd/test_snapshots.txt (1)
95-95: LGTM! Test snapshot entry properly added.The new snapshot file entry follows the existing naming convention and is correctly positioned alphabetically in the list.
src/rpc/mod.rs (1)
101-101: LGTM! RPC method registration is correct.The new
FilecoinAddressToEthAddressmethod is properly registered in the eth vertical section, following the established pattern and logically grouped with the related address conversion method.CHANGELOG.md (1)
38-39: LGTM! Changelog entry is properly formatted.The changelog entry correctly documents the new RPC method with proper formatting, categorization, and references both API v1 and v2 support as expected.
src/rpc/methods/eth.rs (1)
2747-2755: EthAddressToFilecoinAddress DESCRIPTION matches behaviorAdding the DESCRIPTION clarifies that this method specifically returns f410-style Filecoin addresses for Ethereum accounts, without changing behavior or API surface.
| FilecoinAddressToEthAddress::request(( | ||
| "t410f744ma4xsq3r3eczzktfj7goal67myzfkusna2hy" | ||
| .parse() | ||
| .unwrap(), | ||
| None, | ||
| )) |
There was a problem hiding this comment.
Let's add all supported categories of addresses to tests, f0-f4.
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 (2)
src/tool/subcommands/api_cmd/api_compare_tests.rs(2 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tool/subcommands/api_cmd/test_snapshots.txt
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.350Z
Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.
📚 Learning: 2025-09-02T10:05:34.350Z
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.350Z
Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.
Applied to files:
src/tool/subcommands/api_cmd/api_compare_tests.rs
⏰ 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). (7)
- GitHub Check: tests
- GitHub Check: tests-release
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
🔇 Additional comments (3)
src/tool/subcommands/api_cmd/api_compare_tests.rs (3)
1459-1463: LGTM: Cleaner parsing syntax.Using
.parse()is more idiomatic than explicitly callingfrom_str(), and the behavior is identical.
1947-1955: LGTM: Good addition of miner address conversion test.Testing the address conversion with real miner addresses from blocks provides good coverage of ID addresses (f0) in actual chain scenarios.
1959-1967: LGTM: Good addition of message sender address conversion tests.Testing address conversion with actual message senders from the chain provides coverage of various address types used in real transactions.
| FilecoinAddressToEthAddress::request(( | ||
| "t410f744ma4xsq3r3eczzktfj7goal67myzfkusna2hy" | ||
| .parse() | ||
| .unwrap(), | ||
| None, | ||
| )) | ||
| .unwrap(), | ||
| )); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add tests for all address types (f0-f3) as requested in the previous review.
The test currently only covers f4 (delegated) addresses. Per the previous review feedback, please add explicit test cases for all supported Filecoin address types:
- f0/t0: ID address (e.g.,
t01234or useAddress::new_id(...)) - f1/t1: secp256k1 address (e.g.,
t1w2zb5a723izlm4q3khclsjcnapfzxcfhvqyfoly- already available inprefunded_wallets) - f2/t2: Actor address (e.g.,
t2nfplhzpyeck5dcc4fokj5ar6nbs3mhbdmq6xu3q- already available inprefunded_wallets) - f3/t3: BLS address (e.g.,
t3wmbvnabsj6x2uki33phgtqqemmunnttowpx3chklrchy76pv52g5ajnaqdypxoomq5ubfk65twl5ofvkhshq- already available inprefunded_wallets)
Based on past review comment.
🤖 Prompt for AI Agents
In src/tool/subcommands/api_cmd/api_compare_tests.rs around lines 1465-1472 the
test only covers an f4 (delegated) Filecoin address; add explicit test cases for
the remaining address types f0/t0 (ID), f1/t1 (secp256k1), f2/t2 (actor), and
f3/t3 (BLS). For f0 use Address::new_id(...) or a literal like "t01234". For f1,
f2 and f3 use the existing prefunded_wallets examples (e.g., t1w2zb5...,
t2nfplh..., t3wmbvna...), calling FilecoinAddressToEthAddress::request(...) for
each and asserting the expected results (matching the pattern used for f4),
handling unwraps/errors consistently with the test style.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the commit referenced by the user
git show c5e17b0e3249514937b9cfb2aca1453c3b15082f --stat
Length of output: 312
🏁 Script executed:
#!/bin/bash
# Show the actual changes in that commit
git show c5e17b0e3249514937b9cfb2aca1453c3b15082f
Length of output: 2438
🏁 Script executed:
#!/bin/bash
# Search for all FilecoinAddressToEthAddress test usages in the test file
rg -n "FilecoinAddressToEthAddress::request" src/tool/subcommands/api_cmd/api_compare_tests.rs -A 3 -B 1
Length of output: 773
@hanabi1224, Thanks for pointing out the commit! I can see that you've added coverage for f0 addresses through block.miner_address in eth_tests_with_tipset(), and additional coverage through msg.from() which would exercise various address types based on the snapshot data.
However, the original review comment was requesting explicit test cases for each address type (similar to the explicit f4 test in eth_tests()), rather than data-driven tests that depend on snapshot content. The rationale for explicit tests is:
- Deterministic coverage - Ensures all address types are tested regardless of snapshot content
- Clear documentation - Makes it obvious which address types are supported
- Easier debugging - When a specific address type fails, it's immediately clear which one
The eth_tests_with_tipset() tests you added provide good real-world coverage, but they're dependent on what addresses happen to be in the snapshot. For complete explicit coverage, you could add tests like:
// In eth_tests() function
for addr in [
Address::new_id(1234), // f0
Address::from_str("t1w2zb5a723izlm4q3khclsjcnapfzxcfhvqyfoly").unwrap(), // f1
Address::from_str("t2nfplhzpyeck5dcc4fokj5ar6nbs3mhbdmq6xu3q").unwrap(), // f2
Address::from_str("t3wmbvnabsj6x2uki33phgtqqemmunnttowpx3chklrchy76pv52g5ajnaqdypxoomq5ubfk65twl5ofvkhshq").unwrap(), // f3
] {
tests.push(RpcTest::identity(
FilecoinAddressToEthAddress::request((addr, None)).unwrap()
));
}Would you like to add these explicit tests, or do you feel the data-driven coverage is sufficient for your needs?
🧬 Code Graph Analysis Results
[src/tool/subcommands/api_cmd/api_compare_tests.rs] - Potentially relevant snippets
- RpcTest struct and its run method
- Description: Defines the test to perform against Forest/Lotus RPC endpoints, including request, how to validate syntax/semantics, and the execution flow that records results.
- Purpose: Central to understanding how tests are constructed, executed in parallel, and how results are captured for reporting in the API compare workflow.
- Key aspects included:
- Fields: request, check_syntax, check_semantics, ignore, policy_on_rejected, sort_policy
- Methods: basic_raw, basic, identity, validate, validate_raw, run
- Execution flow: measures duration, performs forest and lotus calls, applies syntax/semantics checks, optionally sorts JSON, builds TestResult with optional TestDump
Note: This block is substantial; the essential behavior is captured by this snippet, which governs how each RPC test is defined and executed in the compare pipeline.
- Test evaluation and result helpers
-
Summary of evaluate_test_success
- Parameters: test_result (TestResult), test (RpcTest), test_criteria_overrides
- Returns: bool indicating whether the test is considered successful given forest/lotus results and policy
- Behavior:
- If both sides are Valid => true
- If both time out => matches overrides (ValidAndTimeout, TimeoutAndTimeout)
- If both are Rejected => policy_on_rejected (Pass, PassWithIdenticalError, PassWithIdenticalErrorCaseInsensitive, PassWithQuasiIdenticalError)
- Otherwise => false
-
Summary of dump_test_data
- Parameters: dump_dir (Path), success (bool), test_dump (TestDump)
- Returns: Ok(())
- Behavior: Writes per-test JSON dump into a subdir named "valid" or "invalid" under dump_dir, with a filename derived from the test method and timestamp
- run_tests orchestration (high level)
- Summary:
- Spawns test executions with a concurrency limit (semaphore)
- Applies test filtering based on a FilterList and optional API path/version filters
- Collects results, tracks per-test outcome via ReportBuilder
- Optionally dumps test data, respects fail-fast on first failure
- Produces a final summary/report and can save to a directory if configured
- Related context helpers (conceptual)
-
TestSummary enum and conversion from errors
- Purpose: Normalize various RPC failure modes into a compact summary (MissingMethod, Rejected, NotJsonRPC, InfraError, BadJson, CustomCheckFailed, Timeout, Valid)
- from_err mapping covers several ClientError variants to these categories
-
TestDump and TestResult
- Purpose: Carry per-test diagnostic data (requests/responses, duration, and optional dumps) to support debugging when tests fail or are inspected post-run
- Note on how these pieces connect to the user comment
- The user comment mentions a specific F0 coverage in a Forest PR commit. The RpcTest framework here is the mechanism by which API compatibility tests are defined and executed between Forest and Lotus within api_compare_tests.rs. The above snippets capture the core testing primitives (RpcTest, result handling, and test execution flow) that would be involved in evaluating or reproducing how F0-like cases are exercised in the compare tests.
If you want exact excerpts, I can paste the complete blocks for:
- RpcTest struct and impl (run method)
- evaluate_test_success
- dump_test_data
- run_tests (core loop and orchestration)
Would you like me to include those full blocks with file path and approximate line references?
🧠 Learnings used
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5944
File: src/chain/store/index.rs:0-0
Timestamp: 2025-08-18T03:09:47.932Z
Learning: In Forest's tipset_by_height caching implementation, hanabi1224 prefers performance-conscious solutions that leverage finality guarantees rather than expensive chain walking for fork detection. The approach of constraining cache lookups to finalized epochs (using CHECKPOINT_INTERVAL >= CHAIN_FINALITY) provides fork safety without the performance cost of ancestry verification.
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.350Z
Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.
There was a problem hiding this comment.
@hanabi1224 I think the AI has a good point here. I'd rather we have explicit test cases.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@LesnyRumcajs The snapshot test is deterministic.
filecoin_filecoinaddresstoethaddress_1765291874419799.rpcsnap.json.zst
{
"request": {
"method_name": "Filecoin.FilecoinAddressToEthAddress",
"params": [
"f410f744ma4xsq3r3eczzktfj7goal67myzfkusna2hy"
]
},
...
}filecoin_filecoinaddresstoethaddress_1765357908887069.rpcsnap.json.zst
{
"request": {
"method_name": "Filecoin.FilecoinAddressToEthAddress",
"params": [
"t04040",
"latest"
]
},
...
}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
build.rs (1)
63-66: LGTM! Comment stripping aligns with test_snapshot.rs.The preprocessing logic is correct and matches the implementation in
test_snapshot.rs, enabling inline comments in test snapshot entries.Optional: Line 67's
!l.starts_with('#')check is now redundant.After the split-and-trim operation on lines 63-66, any line starting with
'#'becomes an empty string, so the!l.starts_with('#')condition on line 67 will never match. The!l.is_empty()check alone is sufficient. However, keeping it is harmless and serves as defensive coding.If you prefer to simplify:
- .filter(|l| !l.is_empty() && !l.starts_with('#')) + .filter(|l| !l.is_empty())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
build.rs(1 hunks)src/tool/subcommands/api_cmd/test_snapshot.rs(1 hunks)src/tool/subcommands/api_cmd/test_snapshots.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tool/subcommands/api_cmd/test_snapshots.txt
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: akaladarshi
Repo: ChainSafe/forest PR: 5923
File: src/rpc/registry/actors/miner.rs:221-223
Timestamp: 2025-09-02T10:05:34.350Z
Learning: For miner actor ChangeOwnerAddress and ChangeOwnerAddressExported methods: versions 8-10 use bare Address as parameter type, while versions 11+ use ChangeOwnerAddressParams. This reflects the evolution of the Filecoin miner actor parameter structures across versions.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5930
File: build.rs:64-77
Timestamp: 2025-08-13T09:43:20.301Z
Learning: hanabi1224 prefers hard compile-time errors in build scripts rather than runtime safeguards or collision detection, believing it's better to fail fast and fix root causes of issues like malformed snapshot names.
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 6057
File: src/cli/subcommands/f3_cmd.rs:0-0
Timestamp: 2025-09-09T10:37:17.947Z
Learning: hanabi1224 prefers having default timeouts (like 10m for --no-progress-timeout) to prevent commands from hanging indefinitely, even when the timeout flag isn't explicitly provided by users. This fail-fast approach is preferred over requiring explicit flag usage.
📚 Learning: 2025-08-25T13:35:24.230Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5969
File: src/tool/subcommands/snapshot_cmd.rs:412-412
Timestamp: 2025-08-25T13:35:24.230Z
Learning: In src/tool/subcommands/snapshot_cmd.rs, the +1 in `last_epoch = ts.epoch() - epochs as i64 + 1` fixes an off-by-1 bug where specifying --check-stateroots=N would validate N+1 epochs instead of N epochs, causing out-of-bounds errors when the snapshot contains only N recent state roots.
Applied to files:
src/tool/subcommands/api_cmd/test_snapshot.rs
📚 Learning: 2025-08-07T13:39:15.107Z
Learnt from: hanabi1224
Repo: ChainSafe/forest PR: 5886
File: interop-tests/src/tests/go_app/gen.go:29-29
Timestamp: 2025-08-07T13:39:15.107Z
Learning: Auto-generated files like those created by rust2go (indicated by "Generated by rust2go. Please DO NOT edit this C part manually." comment) in the Forest project should be skipped during code review as they are not intended for manual editing.
Applied to files:
build.rs
⏰ 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). (7)
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: All lint checks
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: Build MacOS
🔇 Additional comments (1)
src/tool/subcommands/api_cmd/test_snapshot.rs (1)
263-264: LGTM! Inline comment support added correctly.The preprocessing logic correctly strips inline comments from test snapshot entries before pattern matching. The
split("#").next().unwrap()is safe sincesplitalways returns at least one element.
Summary of changes
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes #6309
Other information and links
Change checklist
Summary by CodeRabbit
New Features
Tests
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.