Tx: get rid of BaseTransaction #3744
Conversation
jochem-brouwer
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
…onorepo into tx-remove-basetx
| * @param bits Number of bits to check (64 or 256) | ||
| * @param cannotEqual Pass true if the number also cannot equal one less the maximum value | ||
| */ | ||
| export function valueBoundaryCheck( // TODO: better method name |
There was a problem hiding this comment.
For this and the other util methods:
In the context of composition rather than inheritance, I feel like we should be a bit more explicit with the naming (e.g. with reference to "sharedTxConstructor" rather than "shared constructor"). I'm not sure if this method (and the other method utils) are intended to be used outside of a transaction context, but if they are exported outside of the package, I think we have be particularly explicit with naming. This is actually one of the benefits of class-based methods imo (not that I don't prefer this pattern), is they are naturally namespaced by virtue of the class they belong to.
There was a problem hiding this comment.
These are not supposed to be exported outside of the package! These are only supposed to be used internally. These "shared" methods nevertheless have to be named differently. We also have the opportunity now (due to breaking releases) to give everything a more clear name!
| tx.common = getCommon(opts.common) | ||
| tx.common.updateParams(opts.params ?? paramsTx) | ||
|
|
||
| validateNotArray(txData) // is this necessary? |
There was a problem hiding this comment.
As I recall, this got added because there was some weird edge case where we were accepting txData where somehow an array got passed in.
Part of #3733
This PR is "phase 1" of the big tx refactor. This PR:
src/basetransaction.jsThis PR does not intend to change any logic. Besides that, this PR:
AccessList2930TransactiontoAccessList2930TxandEOACode7702TransactiontoEOACode7702Tx. This closes Tx: Class names not fully consistent yet #3723The current state is still WIP, need to: