Skip to content

Two New FRCs: Wallet-Signing User Stories and EIP-191-equivalent-and-extension (references the user stories) #1127

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

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

bumblefudge
Copy link

Hey all:

I got pulled into a discussion thread on the Lotus repo discussions about wallet-signing safety and EVM/FVM/FEVM, and as promised here are the FRCs for clarity/ground-truth. Hope it helps!

@bumblefudge
Copy link
Author

I'm not sure whether people would want this in scope for the User Stories FRC, but riba just pointed out that non-interactive authN based on signatures over arbitrary payloads is ALSO happening in various places via API and HTTP headers:

If people would like this included for future reference/upgrade/documentational clarity, I'd be glad to add a section?

Using this version byte in FVM contexts is discouraged, but in FEVM contexts, it may be used safely, as FEVM messages and signatures are entirely isomorphic to the EVM.

Note that in the base `0x45` structure defined in [ERC-191], there is no envelope-level affordance for including a `chainId` (314 for Filecoin mainnet, 314159 for Filecoin testnet, 314XXX for L2s and private FEVM networks) to protect against crosschain replay attacks.
For this reason, it is recommended to include this safety mechanism (and others like domain-binding or TTL or nonce) inside the payload, as is required by the standard [Sign-In with Ethereum][ERC-4361] template.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this raises the question for me that as long as you're defining a new 0x46, why not also add this in since you're starting from scratch? 0x19 0x45 <ilecoin Signed Message:\n" + len(message) + chainId> <data to sign>

Copy link
Author

@bumblefudge bumblefudge Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for compatibility reasons! Someone using the Metamask Snap or any other ethereum wallet that simply adds the FEVM chainId would produce messages with the classic thereum Signed Message prefix. Unless I misunderstood the question? duh

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think @rvagg is saying that for native filecoin we dont need to follow strictly 191 and can just add the chainId in the there.

on the other hand we only really have mainnet and testnet, all the others are FEVM which need to follow strict 191 for compat.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it's a good point. if no one has a counterargument, I will update the spec to match the above

@rvagg
Copy link
Member

rvagg commented Feb 18, 2025

👍 I assume from @hugomrdias' name on this that it's a collective summation of the agreement you've reached on this topic? Seems fine to me sans some minor nits in there. There's some sections left blank at the bottom but I'm not sure there's much to say on those topics.

Let's assign this one the number 0102 - update the FIP doc with this and stick an entry in the README pointing to it and we'll work on getting it merged.

@rvagg
Copy link
Member

rvagg commented Feb 18, 2025

Eh, I somehow missed that there are two files in here and only noticed after I saw the title again with "Two"!

I'm not sure what to do with the second file, it seems to me to either be part of the main FRC or moved into the resources/ directory where it can be free-form, but it doesn't look like an FRC.

@bumblefudge
Copy link
Author

bumblefudge commented Feb 18, 2025

Oh, sorry, I had the user stories as a separate FRC file because some industrious future worker-bee might want to add a new FRC to address the EIP-712-equivalent use-case, or any other future signing-UX FRCs might just want to refer to or extend the Use-Cases as a free-standing doc. Is this the first "informational FRC" that provides context but no interfaces for wallets? @hugomrdias do you have strong feelings either way? I can just incorporate it into the main text of FRC-0102 or as a /resources/user-stories.md type exhibit, whatever you think makes more sense.

@hugomrdias
Copy link
Contributor

i will start writing code and tests for this in iso-filecoin

@rvagg im going to add support for chainId in the prefix to start with.

issue: hugomrdias/filecoin#212

@eshon
Copy link

eshon commented Mar 19, 2025

@rvagg - What else is needed here or what are the next steps in the FRC process to get this over the finish line?

Hugo has started implementation in JS.

Copy link
Contributor

@dannyob dannyob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, one XXX to be fixed (unless this is a forward reference to a future FIP, which we may want to label it as for now).

My other change is a suggestion (convert the FRC102bis into an appendix by stripping out metadata and extra sections), but I'm not too fussed on this.

@dannyob
Copy link
Contributor

dannyob commented Mar 19, 2025

@eshon with me and @rvagg I think we can get this over the line soon!

@rvagg
Copy link
Member

rvagg commented Apr 1, 2025

Content is pretty much good to go, @bumblefudge are you able to address the few items in here? Note particularly the lack of a need for a template in the appendix, format it as you want and remove the bits that don't help what you're trying to achieve.

Sorry for being slow on this one, my head has been elsewhere.

@rvagg
Copy link
Member

rvagg commented Apr 2, 2025

Still needs an entry in the README.md too btw.

@bumblefudge bumblefudge requested a review from dannyob April 2, 2025 13:51
@bumblefudge
Copy link
Author

Should be ready to go now? thanks y'all

@rvagg
Copy link
Member

rvagg commented Apr 2, 2025

good, aside from the undeleted resource/fip-0102/user-stories and the bad merge line in README (duplicate of 0098 with the old status)

@rvagg
Copy link
Member

rvagg commented Apr 7, 2025

@bumblefudge you just have to ~identical files now, resources/fip-0102/user-stories.md and resources/frc-0102/user-stories.md. I think you just want to git rm resources/fip-0102/user-stories.md

@bumblefudge
Copy link
Author

well color me mortified. the vestige should be deleted now. this is why "allow repo admins to commit directly to this PR" should be checked by default!

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dannyob want to give this a once over too? looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants