fevm: address #9617 review comments (FEVM nv18 merge) #9984
Conversation
raulk
force-pushed
the
raulk/review-nv18-fevm
branch
from
2741c03 to
08b978c
Compare
We now create the 0x0 Eth null address on genesis as an EthAccount, so using the 0x0 Eth null address is guaranteed to work.
1. Move contract deployment and invocation commands to the "eth" command group. 2. Remove the hack to use EAM#Create2 for an external contract creation. 3. Remove code made stale as a result of these refactors.
raulk
changed the title
chain/signatures.go
Outdated
| } | ||
|
|
||
| if err := sigs.Verify(&msg.Signature, signer, digest); err != nil { | ||
| return xerrors.Errorf("secpk message %s has invalid signature: %w", msg.Cid(), err) |
There was a problem hiding this comment.
not necessarily secp here
chain/gen/genesis/genesis_eth.go
Outdated
| func MakeEthNullAddressActor(av actorstypes.Version, addr address.Address) (*types.Actor, error) { | ||
| actcid, ok := actors.GetActorCodeID(av, manifest.EthAccountKey) | ||
| if !ok { | ||
| return nil, xerrors.Errorf("failed to get account actor code ID for actors version %d", av) |
There was a problem hiding this comment.
| return nil, xerrors.Errorf("failed to get account actor code ID for actors version %d", av) | |
| return nil, xerrors.Errorf("failed to get ethaccount actor code ID for actors version %d", av) |
| return nil, xerrors.Errorf("failed to load init actor: %w", err) | ||
| } | ||
|
|
||
| initState, err := init_.Load(adt.WrapStore(ctx, st.Store), initAct) |
There was a problem hiding this comment.
Instead of manipulating the init actor state directly, you could invoke the actual constructor method since you have a VM here.
There was a problem hiding this comment.
EthAccounts cannot be constructed via the Init actor.
They have no public interface: https://github.com/filecoin-project/FIPs/blob/master/FIPS/fip-0055.md#actor-interface
They only have a private interface exposed to the system: https://github.com/filecoin-project/builtin-actors/blob/ad141a1d8998e3fb0d3122e677352dd9c7462184/actors/ethaccount/src/lib.rs#L28 (which I should probably document in the FIP).
chain/types/actor.go
Outdated
| @@ -26,7 +26,7 @@ type ActorV5 struct { | |||
| Head cid.Cid | |||
| Nonce uint64 | |||
| Balance BigInt | |||
| // Predictable Address | |||
| // Deterministic Address: f1, f3, or f4 address. | |||
There was a problem hiding this comment.
This can only be an f4 address (...for now)?
There was a problem hiding this comment.
Correct. We had discussed an alternative version of the world where we hoisted key addresses here, but we didn't go through with that.
cli/chain.go
Outdated
| @@ -68,8 +63,8 @@ var ChainCmd = &cli.Command{ | |||
| ChainEncodeCmd, | |||
| ChainDisputeSetCmd, | |||
| ChainPruneCmd, | |||
| ChainExecEVMCmd, | |||
| ChainInvokeEVMCmd, | |||
| EthDeployCmd, | |||
There was a problem hiding this comment.
I think these can be removed, since they've moved to their new home?
cli/state.go
Outdated
| @@ -776,7 +776,7 @@ var StateGetActorCmd = &cli.Command{ | |||
| fmt.Printf("Nonce:\t\t%d\n", a.Nonce) | |||
| fmt.Printf("Code:\t\t%s (%s)\n", a.Code, strtype) | |||
| fmt.Printf("Head:\t\t%s\n", a.Head) | |||
| fmt.Printf("Predictable address:\t\t%s\n", a.Address) | |||
| fmt.Printf("Deterministic address:\t\t%s\n", a.Address) | |||
There was a problem hiding this comment.
I think this should stay as Predictable
There was a problem hiding this comment.
Nope, we moved away from that term in favour of Delegated.
There was a problem hiding this comment.
I used Deterministic because I wrongly thought this could also carry an f1 or f3 address, in addition to the f4.
itests/fevm_test.go
Outdated
| @@ -114,7 +114,7 @@ func TestFEVMETH0(t *testing.T) { | |||
| av, err := actorstypes.VersionForNetwork(nv) | |||
| require.NoError(t, err) | |||
|
|
|||
| evmCodeCid, ok := actors.GetActorCodeID(av, manifest.EvmKey) | |||
| evmCodeCid, ok := actors.GetActorCodeID(av, manifest.EthAccountKey) | |||
There was a problem hiding this comment.
Rename the variable (or better yet switch to using AssertActorType)
Related Issues
#9617
Closes filecoin-project/ref-fvm#1173.
Proposed Changes
Check commit log for individual changes included here.
Additional Info
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>fix: mempool: Introduce a cache for valid signaturesPR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps