-
Notifications
You must be signed in to change notification settings - Fork 170
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
FIP-0055: Ethereum accounts, addresses, and signatures (broken out of FIP-0054) #580
Conversation
@jennijuju @kaitlin-beegle can I have a FIP number here, please? |
FIPS/fip-nnnn-eam-eth-accounts.md
Outdated
@@ -0,0 +1,386 @@ | |||
--- | |||
fip: <to be assigned> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0055
Is this ready for editor review? |
Yes, please move through. FIP number incorporated. |
This FIP draft is ready for merge and should be close to Last Call quality. |
FIPS/fip-0055.md
Outdated
fip: "0055" | ||
title: Supporting Ethereum Accounts, Addresses, and Transactions | ||
author: Raúl Kripalani (@raulk), Steven Allen (@stebalien) | ||
discussions-to: <URL> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's not a clear need for this FIP to have its own discussion post, except that there ought to be a landing place for someone who wants to ask questions about this particular spec, etc.
Unless you want to open up a distinct comment thread, let's just direct them here: #287
Also happy if you want to link them to other discussions about the f4 address class or FVM actors; no preference on my end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've linked to these three discussion threads here:
(im only half way through and will finish 👁️ tmr morning. @raulk could you please confirm if this PR is thorough review read in preparation of last call, or its just a draft update? |
Feel free to do a Core Dev review in addition to a FIP Editor review here since my understanding is that we're pushing this over to Last Call imminently. But please separate both reviews as FIP Editor review may be blocking for merge, and Core Dev review might not be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broadly looks good to me, various suggestions, only a couple blockers.
As a general note, this FIP is a little too forward-looking in terms of Account Abstraction. I think I understand the motivation for this approach to the FIP, and that future enhancement is germane to the design & history of this FIP, but it would be nicer if one could read & understand the specification of the immediate proposed change without thinking too much about future changes.
On an aside, it's really helpful for folks implementing migrations for network upgrades if a FIP specifies all necessary migration changes in a single section. This isn't especially relevant to this FIP (the migration is relatively simple, and has already been implemented in Golang), but I think it might be a good idea to modify the FIP template to add an explicit section for migrations. Note for future discussion.
FIPS/fip-0055.md
Outdated
|
||
1. The Wasm bytecode of the EAM (referred by its CodeCID) is linked under the System actor's registry under the key `"eam"`. | ||
2. A single copy of the EAM is deployed as a singleton actor under ID address `f010`. | ||
3. The EAM is allowed-listed in the Init actor as an _authorized_ delegated address manager. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BLOCKER: I might be wrong, but I think this doesn't actually happen as part of the migration? Instead, the new Init Actor has the EAM pre-approved as an authorized delegated address manager.
Can resolve by either dropping this line, or telling me I'm wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the discrepancy lies in that you interpreted this as an explicit action (e.g. there is some kind of transaction that happens after which the EAM becomes authorized), whereas I intended to write this as a constraint of the code. I can see why there's confusion here. Let me rejigger this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried to bring more accuracy to this in 007f126.
FIPS/fip-0055.md
Outdated
|
||
#### State | ||
|
||
This actor has no state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this a bit more explicit -- the Head
of the EAM is set to the EMPTY_ARR_CID
.
We also might want to specify that the migration should explicitly add this object to the state store? I'm not sure whether that's necessary, but I did so in the current Golang implementation of the migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 682e0f7.
FIPS/fip-0055.md
Outdated
|
||
#### State | ||
|
||
None. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar note about State
as for the EAM above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 682e0f7.
|
||
None. | ||
|
||
## Design Rationale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the upgrade path towards "full" AA is the only relevant design rationale to discuss. At the very least, I think we should reiterate (or extract and move here) some of the salient points under the Change Motivation section about wanting to to retain full Ethereum tooling and ecosystem stack compatibility with Filecoin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that, but discarded it to avoid repetition. Can do if folks really want that repetition here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've had similar uncertainty when writing FIPs myself -- there's often major overlap between the Change Motivation and the Design Rationale (with the latter coming down to "designed so as to achieve the motivated change".
Defer to you / anyone else who feels strongly one way or the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any specific design decisions you would like to call out here? If not, let's leave it as-is. The general address manager stuff should be in FIP-0048 (the one that introduces the notion of address managers), and I don't think there are any other noteworthy decisions specific to the EthAccount actor here beyond AA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with avoiding repetition. I interpret these sections as:
- Motivation: what's the problem and why is it worth solving at all
- Design rationale: why solve it this way, instead of some other ways
As a technical reviewer, I would like to see more explanation of why promotion is done this way, why it can't be done a more general way from the beginning. The section could probably highlight each of the Ethereum-specific mechanisms proposed (e.g. also the transitory signature delegation), explain they are temporary, and explain why the shortcut was needed.
FIPS/fip-0055.md
Outdated
|
||
## Backwards Compatibility | ||
|
||
Backwards compatibility is not affected, as this FIP introduces strictly additive features. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIP authors usually discuss the need for a network upgrade in this section (see a very simple example here). TBH, I'm not sure this is especially useful, since I don't think we've ever had a FIP that didn't need a network upgrade, but you can match precedent if you'd like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the relationship between a network upgrade and backwards compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a Migration section which covers the transition during the network upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's "backwards incompatible" in that it's "v9 incompatible", and so needs new actors version / network version -- that's often all this section has come down to.
Don't think you should necessarily match precedent here, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 75b22a6.
FIPS/fip-0055.md
Outdated
|
||
## Implementation | ||
|
||
- The actor implementation lives in [`filecoin-project/builtin-actors`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we link the appropriate branches here, with a note that they will land in main branches upon this FIP's acceptance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
FIPS/fip-0055.md
Outdated
discussions-to: | ||
- https://github.com/filecoin-project/FIPs/discussions/388 | ||
- https://github.com/filecoin-project/FIPs/discussions/490 | ||
- https://github.com/filecoin-project/FIPs/discussions/287 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we narrow this down to a single venue, perhaps #490, so technical discussion related to this proposal can be found in one place?
|
||
None. | ||
|
||
## Design Rationale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with avoiding repetition. I interpret these sections as:
- Motivation: what's the problem and why is it worth solving at all
- Design rationale: why solve it this way, instead of some other ways
As a technical reviewer, I would like to see more explanation of why promotion is done this way, why it can't be done a more general way from the beginning. The section could probably highlight each of the Ethereum-specific mechanisms proposed (e.g. also the transitory signature delegation), explain they are temporary, and explain why the shortcut was needed.
|
||
### Exchange adaptation | ||
|
||
Exchanges must support receiving funds from EVM smart contracts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some don't even support receiving funds from native built-in actors like a multisig. There's not much we can do about it. I suggest dropping this, or at least softening to just being one more thing that exchanges might want to consider.
The original signature payload must be reconstructed by repacking the RLP-encoded [EIP-1559 Ethereum transaction]. | ||
Any flaws in this logic is subject to security events. | ||
|
||
### Inability to stably address some actors from EVM smart contracts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this section, but discussion taken to #490 (comment)
|
||
Such transitory `Delegated` signatures must be verified by: | ||
|
||
1. Reforming and repacking the native RLP-encoded [EIP-1559 Ethereum transaction] from the Filecoin message (original signature payload). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EIP-1559 implies EIP-2718 transaction envelopes but you might want to specifically require that to reserve the potential to add more custom transaction types in future; either your own types, or if necessary, upstream Ethereum transaction types.
This FIP breaks out the Ethereum Account (formally EEOA), EAM, Delegated signatures, and Ethereum addresses out of the monolith FIP-0054. This will allow for more focused and efficient community review.
Simple Summary
In order to enable existing Ethereum tools to seamlessly interact with the Filecoin network, it is necessary for Filecoin to:
This FIP delivers these requirements by leveraging the f4 extensible address class (newly introduced in FIP-0048) to model Ethereum Addresses, managed by the Ethereum Address Manager (EAM) actor introduced herein.
It also introduces the Ethereum Account actor to represent Ethereum Externally Owned accounts, capable of acting as message senders, and a new Delegated signature type, which enables validating native EIP-1559 Ethereum transactions carrying an secp256k1 ECDSA signatures over their RLP encoding.
Both these elements act as extension points for an eventual fully-fledged Account Abstraction framework.
Abstract
This FIP introduces three technical elements:
It formally defines the
f410
Ethereum address space, nested under decimal namespace10
within thef4
address class as defined in FIP-0048. This address space is managed by the Ethereum Address Manager (EAM) actor, which also offers public methods to act as a factory for EVM smart contracts. The address payload of anf410
address is the original Ethereum address. Ethereum addresses can be cast asf410
addresses, and viceversa.The Ethereum Account actor, representing an Externally-Owned Ethereum account, backed by a secp256k1 key, and capable of acting as a sender of native EIP-1559 Ethereum transactions. This actor will eventually become an Abstract Account (AA) actor, once such framework is introduced.
The Delegated signature type, carrying a signature eventually verified by actor code through Account Abstraction (AA). At this time, Delegated signatures are special-cased secp256k1 ECDSA signatures over the RLP encoding of native EIP-1559 Ethereum transactions.