feat(wallet)!: apply FRC-0102 envelope to sign/verify by default#7216
Open
0xDevNinja wants to merge 1 commit into
Open
feat(wallet)!: apply FRC-0102 envelope to sign/verify by default#72160xDevNinja wants to merge 1 commit into
0xDevNinja wants to merge 1 commit into
Conversation
0xDevNinja
requested review from
LesnyRumcajs and
hanabi1224
and removed request for
a team
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds FRC-0102 signing envelope support to ChangesFRC-0102 Envelope in forest-wallet
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
6 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/wallet/subcommands/wallet_cmd.rs (2)
810-813: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAvoid direct slicing in tests; use
.get()to prevent panic-style failures.Line 810 through Line 813 use direct range slicing. Switching to
.get()keeps failures explicit and follows the repository Rust safety rule.Suggested patch
- use super::{SignatureType, resolve_method_num, resolve_target_address, wrap_frc0102}; + use super::{ + FRC_0102_FILECOIN_PREFIX, SignatureType, resolve_method_num, resolve_target_address, + wrap_frc0102, + }; @@ - assert_eq!(&wrapped[..26], b"\x19Filecoin Signed Message:\n"); - assert_eq!(&wrapped[26..26 + digits.len()], digits.as_bytes()); - assert_eq!(&wrapped[26 + digits.len()..], msg.as_slice()); + let prefix_len = FRC_0102_FILECOIN_PREFIX.len(); + assert_eq!( + wrapped + .get(..prefix_len) + .expect("wrapped message shorter than FRC-0102 prefix"), + FRC_0102_FILECOIN_PREFIX + ); + let digits_end = prefix_len + digits.len(); + assert_eq!( + wrapped + .get(prefix_len..digits_end) + .expect("wrapped message missing encoded length"), + digits.as_bytes() + ); + assert_eq!( + wrapped + .get(digits_end..) + .expect("wrapped message missing payload"), + msg.as_slice() + );As per coding guidelines,
**/*.rs: "Use.get()instead of direct indexing[index]to avoid panics".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/wallet/subcommands/wallet_cmd.rs` around lines 810 - 813, Replace the direct range slicing operations on the wrapped variable with the safer `.get()` method to avoid panic-style failures. In the test assertions (lines checking wrapped[..26], wrapped[26..26 + digits.len()], and wrapped[26 + digits.len()..]), convert each direct slice access to use `.get()` with appropriate range parameters, and handle the Result type by unwrapping or matching on the returned Option to maintain the assertion logic while following the repository's Rust safety guidelines.Source: Coding guidelines
169-177: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd operation context to local signing errors.
Line 171 and Line 173 propagate errors without local operation context, which makes CLI failures harder to diagnose.
Suggested patch
async fn wallet_sign(&self, address: Address, message: Vec<u8>) -> anyhow::Result<Signature> { if let Some(keystore) = &self.local { - let key = crate::key_management::try_find_key(&address, keystore)?; + let key = crate::key_management::try_find_key(&address, keystore) + .with_context(|| format!("failed to find local key for address {address}"))?; Ok(crate::key_management::sign( *key.key_info.key_type(), key.key_info.private_key(), &message, - )?) + ) + .with_context(|| format!("failed to sign message with address {address}"))?) } else { Ok(WalletSign::call(&self.remote, (address, message)).await?) } }As per coding guidelines,
**/*.rs: "Useanyhow::Result<T>for most operations and add context with.context()when errors occur".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/wallet/subcommands/wallet_cmd.rs` around lines 169 - 177, The wallet_sign function propagates errors from try_find_key and crate::key_management::sign without providing operational context, making CLI failures difficult to diagnose. Add error context using the .context() method to both the try_find_key call (which finds the key by address) and the sign call (which performs the actual signing operation) with descriptive messages that explain what operation was being attempted when the error occurred.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/wallet/subcommands/wallet_cmd.rs`:
- Around line 810-813: Replace the direct range slicing operations on the
wrapped variable with the safer `.get()` method to avoid panic-style failures.
In the test assertions (lines checking wrapped[..26], wrapped[26..26 +
digits.len()], and wrapped[26 + digits.len()..]), convert each direct slice
access to use `.get()` with appropriate range parameters, and handle the Result
type by unwrapping or matching on the returned Option to maintain the assertion
logic while following the repository's Rust safety guidelines.
- Around line 169-177: The wallet_sign function propagates errors from
try_find_key and crate::key_management::sign without providing operational
context, making CLI failures difficult to diagnose. Add error context using the
.context() method to both the try_find_key call (which finds the key by address)
and the sign call (which performs the actual signing operation) with descriptive
messages that explain what operation was being attempted when the error
occurred.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e3acfb8d-f708-4316-81f0-e9990ea3d88a
📒 Files selected for processing (3)
.config/forest.dicCHANGELOG.mdsrc/wallet/subcommands/wallet_cmd.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
filecoin-project/lotus(manual)
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
0xDevNinja
force-pushed
the
0xdevninja/issue-6442-frc0102-wallet
branch
from
0874419 to
2f54633
Compare
|
|
||
| - [#7164](https://github.com/ChainSafe/forest/issues/7164): JSON-RPC authentication is now performed once per connection (e.g. at the WebSocket upgrade) instead of on every request, matching Lotus. Note that token expiry is no longer re-checked for the lifetime of an established connection. | ||
|
|
||
| - [#6442](https://github.com/ChainSafe/forest/issues/6442): `forest-wallet sign` and `forest-wallet verify` now apply the FRC-0102 signing envelope to the message by default. Pass `--raw` on both sides to reproduce the previous raw-bytes behaviour. |
Member
There was a problem hiding this comment.
Let's move that to the current release train
Member
|
LGTM, just please update the changelog. |
`forest-wallet sign` and `forest-wallet verify` now wrap the decoded message with the FRC-0102 Filecoin signing envelope (`\x19Filecoin Signed Message:\n<len>`) before handing it to the signer / verifier. This aligns Forest with the Ledger Filecoin app, Filsnap, iso-filecoin and Lotus (filecoin-project/lotus#13388), so signatures produced by any of these wallets over the same bytes now round-trip. A new `--raw` flag restores the prior raw-bytes behaviour on both commands. `--raw` is also the correct choice when signing on-chain Filecoin messages, which must not be wrapped. The envelope is applied client-side only; the Forest and Lotus `WalletSign` / `WalletVerify` RPCs treat their input as opaque bytes, so a single wrap here is correct against today's daemons. BREAKING CHANGE: signatures produced by the default invocation are not verifiable by pre-FRC-0102 Forest or Lotus releases without `--raw` on the verifier side. Refs ChainSafe#6442
0xDevNinja
force-pushed
the
0xdevninja/issue-6442-frc0102-wallet
branch
from
2f54633 to
04acdf6
Compare
Contributor
Author
|
Done — moved the entry up to the unreleased train. Thanks for the review! |
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
2 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
Reopens the work from #6967, which was closed for inactivity. Rebased on current
mainand the two outstanding review comments are addressed.Changes introduced in this pull request:
forest-wallet signandforest-wallet verifynow apply the FRC-0102 signing envelope (0x19 || "Filecoin Signed Message:\n" || ascii(len(msg)) || msg) to the message by default.--rawflag on both sides reproduces the previous raw-bytes behaviour.wallet_signnow takesVec<u8>and passes the payload through unchanged, so both paths operate on the same bytes).Reference issue to close (if applicable)
Closes #6442
Other information and links
Follow-up from the review on #6967 (thanks @akaladarshi for the guidance there). Addressed both remaining comments:
Filsnapentry from.config/forest.dic(interoperatingis still referenced in a doc comment, kept).wrap_frc0102unit tests into parametrized#[rstest]cases (envelope-shape cases and the length-boundary cases).Change checklist
Summary by CodeRabbit
forest-wallet signandforest-wallet verifynow apply the FRC-0102 signing envelope to messages by default.--rawflag on either command to preserve the previous raw-bytes message behavior.