Use upstream account packages #678
Conversation
darioush
force-pushed
the
use-libevm-account
branch
2 times, most recently
from
e102e7c
to
79bcc2d
Compare
darioush
force-pushed
the
use-libevm-account
branch
from
79bcc2d
to
1635994
Compare
| @@ -426,7 +426,7 @@ type syncVMSetup struct { | |||
| serverAppSender *enginetest.Sender | |||
|
|
|||
| includedAtomicTxs []*Tx | |||
| fundedAccounts map[*keystore.Key]*types.StateAccount | |||
| fundedAccounts map[*utils.Key]*types.StateAccount | |||
There was a problem hiding this comment.
Since one of the fields in utils.Key is a pointer, this doesn't guarantee uniqueness. Is that ok?
There was a problem hiding this comment.
It seems okay as randomly generated keys would likely not be duplicate and this change does not modify this part of the behavior
| @@ -116,7 +116,7 @@ func fillAccountsWithStorage(t *testing.T, serverDB ethdb.Database, serverTrieDB | |||
| // returns the new trie root and a map of funded keys to StateAccount structs. | |||
| func FillAccountsWithOverlappingStorage( | |||
| t *testing.T, trieDB *triedb.Database, root common.Hash, numAccounts int, numOverlappingStorageRoots int, | |||
| ) (common.Hash, map[*keystore.Key]*types.StateAccount) { | |||
| ) (common.Hash, map[*utils.Key]*types.StateAccount) { | |||
| PrivateKey *ecdsa.PrivateKey | ||
| } | ||
|
|
||
| func NewKey(rand io.Reader) (*Key, error) { |
There was a problem hiding this comment.
Why accept the reader if it's always crypto/rand.Reader (at least in the code I can see)? I understand wanting stable keys in tests, but it doesn't look like that's being done and this risks someone may accidentally pass a non-secure source.
There was a problem hiding this comment.
I think this is fine since it's the same in the existing code it was adapted from and also in ecdsa.GenerateKey
Additionally it's recommended to receive interfaces in golang as opposed to concrete types
There was a problem hiding this comment.
receive interfaces in golang as opposed to concrete types
I mean not receive anything at all and just use crypto/rand.Reader inside the function. I'm happy with whatever you decide here—consider my original question as just a prompt to spur thinking.
Why this should be merged
Less code to maintain.
How this works
Since we took types.Transaction, some more interfaces can be unified, and we can drop this code from our codebase.
How this was tested
CI