Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

Add Account class #275

Merged
merged 11 commits into from
Oct 7, 2020
Merged

Add Account class #275

merged 11 commits into from
Oct 7, 2020

Conversation

ryanio
Copy link
Contributor

@ryanio ryanio commented Sep 25, 2020

This PR adds a new Account class intended as a modern replacement for ethereumjs-account. It has a shape of Account(nonce?: BN, balance?: BN, stateRoot?: Buffer, codeHash?: Buffer).

Instantiation

The static factory methods assist in creating an Account object from varying data types: Object: fromAccountData, RLP: fromRlpSerializedAccount, and Array: fromValuesArray.

Methods: isEmpty(): boolean, isContract(): boolean, serialize(): Buffer

Example usage:

import { Account, BN } from 'ethereumjs-util'

const account = new Account(
  new BN(0), // nonce, default: 0
  new BN(10).pow(new BN(18)), // balance, default: 0
  undefined, // stateRoot, default: KECCAK256_RLP (hash of RLP of null)
  undefined, // codeHash, default: KECCAK256_NULL (hash of null)
)

For more info see the documentation or examples of usage in test/account.spec.ts.

@coveralls
Copy link

coveralls commented Sep 25, 2020

Coverage Status

Coverage decreased (-0.2%) to 99.527% when pulling e48478d on add-account-class into d8b8e44 on master.

@@ -6,6 +6,7 @@

### Modules

* ["@types/ethjs-util/index"](modules/__types_ethjs_util_index_.md)
Copy link
Contributor Author

@ryanio ryanio Sep 25, 2020

Choose a reason for hiding this comment

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

I couldn't find a way to ignore this file in typedoc without getting the error Could not find a declaration file for module 'ethjs-util'., so I guess there is no harm in having it for now, although I wish the file was filled with the type info rather than just being empty 😅

The library mode in typedoc is pretty new and works quite well for our purposes, so perhaps it will be fixed or improved in the future:

Warning: File ./src/@types/ethjs-util/index.ts is not a module and cannot be converted in library mode

@ryanio ryanio force-pushed the add-account-class branch 4 times, most recently from 992ee3a to abc6f9f Compare September 25, 2020 23:36
Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

Some comments regarding consistency and explicitness. Looks good in general 👍 😄

src/account.ts Outdated Show resolved Hide resolved
src/account.ts Outdated Show resolved Hide resolved
src/account.ts Show resolved Hide resolved
test/account.spec.ts Show resolved Hide resolved
test/account.spec.ts Outdated Show resolved Hide resolved
test/account.spec.ts Outdated Show resolved Hide resolved
test/account.spec.ts Outdated Show resolved Hide resolved
test/account.spec.ts Outdated Show resolved Hide resolved
src/account.ts Outdated Show resolved Hide resolved
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Somewhat comment-focused review, some suggestions for improvements, otherwise looks good.

src/account.ts Outdated
if (codeHash.length !== 32) {
throw new Error('codeHash must have a length of 32')
}

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add nonce and balance >= 0 checks here as well or is this unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, added in b0aacc0

src/account.ts Show resolved Hide resolved
src/account.ts Show resolved Hide resolved
src/account.ts Show resolved Hide resolved
src/account.ts Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
})
})
})

Copy link
Member

Choose a reason for hiding this comment

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

Really nice tests. 😄

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Thanks Ryan, looks really great! Sorry that this took so long to review, fall a bit aside, feel free to be more pushy/demanding here if you need things for subsequent tasks.

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

Successfully merging this pull request may close these issues.

4 participants