- Notifications
You must be signed in to change notification settings - Fork 276
Add Account class #275
Add Account class #275
Conversation
| | ||
| ### Modules | ||
| | ||
| * ["@types/ethjs-util/index"](modules/__types_ethjs_util_index_.md) |
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 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 992ee3a to abc6f9f Compare
jochem-brouwer left a comment
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 comments regarding consistency and explicitness. Looks good in general 👍 😄
fca1367 to 18e90bf Compare
holgerd77 left a comment
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.
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') | ||
| } | ||
| |
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.
Do we want to add nonce and balance >= 0 checks here as well or is this unnecessary?
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.
sure, added in b0aacc0
| }) | ||
| }) | ||
| }) | ||
| |
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.
Really nice tests. 😄
holgerd77 left a comment
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.
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.
This PR adds a new
Accountclass intended as a modern replacement forethereumjs-account. It has a shape ofAccount(nonce?: BN, balance?: BN, stateRoot?: Buffer, codeHash?: Buffer).Instantiation
The static factory methods assist in creating an
Accountobject from varying data types:Object: fromAccountData,RLP: fromRlpSerializedAccount, andArray: fromValuesArray.Methods:
isEmpty(): boolean,isContract(): boolean,serialize(): BufferExample usage:
For more info see the documentation or examples of usage in
test/account.spec.ts.