feat(wallet)!: apply FRC-0102 envelope to sign/verify by default#6967
feat(wallet)!: apply FRC-0102 envelope to sign/verify by default#69670xDevNinja wants to merge 1 commit into
Conversation
0xDevNinja
requested review from
akaladarshi and
hanabi1224
and removed request for
a team
|
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 conditional FRC-0102 envelope wrapping to ChangesFRC-0102 Envelope Implementation
Sequence DiagramsequenceDiagram
participant CLI as forest-wallet CLI
participant Wrap as wrap_frc0102
participant Backend as WalletBackend
participant Signer as crypto signing/verification
CLI->>CLI: parse message hex, check --raw
CLI->>Wrap: wrap_frc0102(msg) when raw==false
CLI->>Backend: pass message bytes to wallet_sign/wallet_verify
Backend->>Signer: perform signature or verification on provided bytes
Signer-->>Backend: signature/verification result
Backend-->>CLI: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 475-491: The Sign branch is base64-encoding the wrapped message
(BASE64_STANDARD.encode) before sending to backend.wallet_sign, which causes
remote wallets to sign the base64 text bytes instead of the raw/wrapped bytes
used by verify; change the flow in the Self::Sign arm so that
backend.wallet_sign receives the raw byte payload (i.e., the result of
wrap_frc0102(&message) or message when raw is true) rather than the base64
text—move or remove the BASE64_STANDARD.encode call and only base64-encode
when/if the backend API truly expects a base64 string, ensuring
functions/variables mentioned (Self::Sign, wrap_frc0102, BASE64_STANDARD.encode,
backend.wallet_sign) are updated consistently so sign and verify operate on
identical byte sequences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8b670a84-4ce2-4479-bc7e-ef7c7caf0fd7
📒 Files selected for processing (2)
CHANGELOG.mdsrc/wallet/subcommands/wallet_cmd.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Hi @0xDevNinja |
|
Any update on this @0xDevNinja ? |
0xDevNinja
force-pushed
the
0xdevninja/issue-6442-frc0102-wallet
branch
from
24b9f00 to
4514566
Compare
|
Hey, sorry for the delay. Just pushed an update:
Let me know if anything else stands out. |
0xDevNinja
force-pushed
the
0xdevninja/issue-6442-frc0102-wallet
branch
2 times, most recently
from
2a78947 to
c985118
Compare
|
|
||
| /// FRC-0102 envelope prefix for Filecoin `personal_sign`-style messages. | ||
| /// | ||
| /// See <https://github.com/filecoin-project/FIPs/blob/master/FRCs/frc-0102.md>. |
There was a problem hiding this comment.
Link to the FRC should be an internal doc not user facing.
Also keep the comment simple:
| /// See <https://github.com/filecoin-project/FIPs/blob/master/FRCs/frc-0102.md>. | |
| /// FRC-0102 envelope prefix format: `0x19 || "Filecoin Signed Message:\n" || ascii(len(msg)) || msg`. |
There was a problem hiding this comment.
Done — dropped the FRC link, collapsed to the one-line wire-format note. ff406be63
There was a problem hiding this comment.
Don't drop the link keep the FRC link but make it for internal doc and make sure the link refers to a specific commit not master (as it subjected to change).
Also change the comment to something like below and you can remove the comment from const
| /// See <https://github.com/filecoin-project/FIPs/blob/master/FRCs/frc-0102.md>. | |
| /// Wraps `msg` with the FRC-0102 envelope: `0x19 || "Filecoin Signed Message:\n" || ascii(len(msg)) || msg` | |
| // See <https://github.com/filecoin-project/FIPs/blob/bdd5283279fd115c87c9bbf71d2e40c9d075f5aa/FRCs/frc-0102.md>. |
There was a problem hiding this comment.
Done, restored the link pinned to that commit SHA and updated the comment. Also signed commits.
There was a problem hiding this comment.
Quick follow-up — clippy flagged the blank line between /// and //, so I dropped it. Also rebased on main to clear the conflict with the wallet StrictAddress change.
0xDevNinja
force-pushed
the
0xdevninja/issue-6442-frc0102-wallet
branch
from
c985118 to
ff406be
Compare
0xDevNinja
force-pushed
the
0xdevninja/issue-6442-frc0102-wallet
branch
from
ff406be to
11ad84a
Compare
| Filfox | ||
| Filops | ||
| FilOz | ||
| Filsnap |
There was a problem hiding this comment.
It's is not being used anywhere, you can remove this.
| } | ||
|
|
||
| #[test] | ||
| fn test_wrap_frc0102_contains_newline() { |
There was a problem hiding this comment.
You can combine above tests into single test using the rstest and cases. Since they are all roughly checking the same thing.
|
@0xDevNinja Please verify the commits here as well. |
0xDevNinja
force-pushed
the
0xdevninja/issue-6442-frc0102-wallet
branch
from
11ad84a to
3197585
Compare
`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
3197585 to
df8482b
Compare
|
@0xDevNinja Can you please address the remaining last two comments. |
2 participants
Summary of changes
Changes introduced in this pull request:
\x19Filecoin Signed Message:\n<len>) inforest-wallet signandforest-wallet verifybefore handing it to the signer/verifier. Aligns Forest with Ledger Filecoin, Filsnap, iso-filecoin and Lotus (feat(cli): add FRC-102 compatible EIP-191 signing via --fevm flag (#13256) filecoin-project/lotus#13388).--rawflag to bothsignandverifythat restores the previous raw-bytes behaviour.--rawis also the correct choice when signing on-chain Filecoin messages, which must not be wrapped.wrap_frc0102(&[u8]) -> Vec<u8>helper with aconst FRC_0102_FILECOIN_PREFIX: &[u8]inwallet_cmd.rs.secp256k1round-trip that assertsverify(wrapped)succeeds andverify(raw)fails.CHANGELOG.md:Breakingentry that also calls out (a) pre-FRC-0102 nodes cannot verify the new default without--raw, and (b) on-chain messages must still use--raw.The envelope is applied client-side only. Forest's and Lotus's
WalletSign/WalletVerifyRPCs treat their input as opaque bytes and do not wrap, so a single wrap here is correct against today's daemons. If that ever changes server-side, users will want--rawto avoid a double envelope.Reference issue to close (if applicable)
Closes #6442
Other information and links
sign/verifywrap by default +--rawopt-out).Change checklist
Outside contributions
AI Usage Disclosure
This PR was prepared with assistance from Claude Code (Anthropic). Extent:
cargo fmt --all -- --check,cargo clippy --profile quick --all-targets --no-deps -- -D warnings(withFOREST_F3_SIDECAR_FFI_BUILD_OPT_OUT=1), andcargo test --lib -- wrap_frc0102 frc0102_roundtrip(7/7 pass) are all green.wallet_sign/wallet_verifythat receives the already-wrapped bytes.Summary by CodeRabbit
Breaking Changes
--rawis used.New Features
--rawoption to control envelope wrapping for both sign and verify commands.Documentation
Chores