Replace account package with ethereumjs-util Account #911
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
ryanio
force-pushed
the
remove-account-package
branch
4 times, most recently
from
332a47a to
fd108a3
Compare
ryanio
force-pushed
the
remove-account-package
branch
from
fd108a3 to
633255e
Compare
| @@ -1,5 +1,4 @@ | |||
| import BN = require('bn.js') | |||
| import Account from '@ethereumjs/account' | |||
| import { Account, BN } from 'ethereumjs-util' | |||
There was a problem hiding this comment.
There is still a reference to ethereumjs-account left in the code docs of this file (on the step event).
Side note: does the step event also need some attention on this regard? 🤔 At least a mention in the release notes?
There was a problem hiding this comment.
updated in 12ff416, yes I think we should mention in the release notes.
packages/vm/lib/index.ts
Outdated
| balance: '0x01', | ||
| }), | ||
| ), | ||
| this.stateManager.putAccount(address, new Account(new BN(0), new BN(1))), |
There was a problem hiding this comment.
I would suggest to generally avoid to use the main constructor. This makes the code less readable since one can't distinguish on the type of values directly.
| @@ -216,7 +215,7 @@ async function applyBlock(this: VM, block: any, opts: RunBlockOpts) { | |||
| * @param {Block} block | |||
| * @param {RunBlockOpts} opts | |||
| */ | |||
| async function applyTransactions(this: VM, block: any, opts: RunBlockOpts) { | |||
| async function applyTransactions(this: VM, block: Block, opts: RunBlockOpts) { | |||
| @@ -1,4 +1,4 @@ | |||
| import Account from '@ethereumjs/account' | |||
| import { Account } from 'ethereumjs-util' | |||
There was a problem hiding this comment.
Ah ok, so this DOES change the StateManager interface, also something worth to be mentioned in the release notes.
| // EIP-161: "An account is considered empty when it has no code and zero nonce and zero balance." | ||
| return ( | ||
| account.nonce.isZero() && account.balance.isZero() && account.codeHash.equals(KECCAK256_NULL) | ||
| ) |
There was a problem hiding this comment.
Ok, we should I would assume remove the stateRoot check again? 🤔
There was a problem hiding this comment.
yes I am still discussing with @jochem-brouwer but I think that is what we will end up doing. this is still safe to merge in, and once updated in util then I can update here back to account.isEmpty()
| nonce: 0, | ||
| balance: new BN(10).pow(new BN(19)), // 10 eth | ||
| } | ||
| const account = Account.fromAccountData(acctData) |
| * @property {Buffer} address the address of the `account` | ||
| * @property {Number} depth the current number of calls deep the contract is | ||
| * @property {Buffer} memory the memory of the VM as a `buffer` | ||
| * @property {BN} memoryWordCount current size of memory in words | ||
| * @property {StateManager} stateManager a [`StateManager`](stateManager.md) instance (Beta API) | ||
| * @property {StateManager} stateManager a [[StateManager]] instance |
There was a problem hiding this comment.
Had to look up these symbol references (double square brackets) - so linking to internal classes, members or functions, really nice functionality! 😄 Just dropping here for others as a reference.
|
Absolutely no criticism, but just realizing what has been forgotten here: switching to our new CHANGELOG procedure and updating the release notes along the way. 😋 Just as some general reminder, think we are all not yet completely in this mode and this will likely take some time to settle in. 😀 |
This PR replaces the account package in the monorepo for the new Account class in ethereumjs-util.
This PR also updates some types and removes redundant
new BN()syntax if the value is already a BN.