defaultTransactionTypeParser Refactor - #6102
Conversation
Deploying with
|
| Latest commit: |
7f676c9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d3df5286.web3-js-docs.pages.dev |
| Branch Preview URL: | https://wyatt-4-x-6065-transaction-t.web3-js-docs.pages.dev |
Bundle StatsHey there, this message comes from a github action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## 4.x #6102 +/- ##
==========================================
- Coverage 87.94% 87.41% -0.53%
==========================================
Files 199 199
Lines 7572 7612 +40
Branches 2055 2063 +8
==========================================
- Hits 6659 6654 -5
- Misses 913 958 +45
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
The types for these tests were updated to 0x0 because they all use gasLimit
| // undefined is treated as null for JSON schema validator | ||
| const type0x0TransactionSchema = { | ||
| type: 'object', | ||
| properties: { | ||
| accessList: { | ||
| type: 'null', | ||
| }, | ||
| maxFeePerGas: { | ||
| type: 'null', | ||
| }, | ||
| maxPriorityFeePerGas: { | ||
| type: 'null', | ||
| }, | ||
| }, | ||
| }; | ||
| const type0x1TransactionSchema = { | ||
| type: 'object', | ||
| properties: { | ||
| maxFeePerGas: { | ||
| type: 'null', | ||
| }, | ||
| maxPriorityFeePerGas: { | ||
| type: 'null', | ||
| }, | ||
| }, | ||
| }; | ||
| const type0x2TransactionSchema = { | ||
| type: 'object', | ||
| properties: { | ||
| gasPrice: { | ||
| type: 'null', | ||
| }, | ||
| }, | ||
| }; | ||
|
|
There was a problem hiding this comment.
I don't believe these should go in schemas.ts since they aren't complete schemas of the transaction object
There was a problem hiding this comment.
just tiny suggestion, if its Tx type schema like ( TypeSchemaFor0x1Tx , TypeSchemaFor0x2Tx ) it will more readable and can be moved to schema file.
There was a problem hiding this comment.
@jdevcs
I'll rename them to: transactionType0x0Schema, but I don't think they should be moved to schemas.ts because then they'll be exported from web3-eth package and these type shouldn't be any where else as they are an incomplete representation of what a typed transaction is (i.e. they don't contain the other fields of a transaction)
I experimented with doing something like:
export const transactionType0x0Schema = {
type: 'object',
properties: {
...transactionSchema.properties,
accessList: {
type: 'null',
},
maxFeePerGas: {
type: 'null',
},
maxPriorityFeePerGas: {
type: 'null',
},
}
};but now the function validateTxTypeAndHandleErrors in defaultTransactionTypeParser is validating the entire transaction object which is out of scope of defaultTransactionTypeParser. This function is only concerned about returning the detected tx type, or throwing an error for an invalid tx object regarding types and their appropriate properties, not the rest of the tx object
I think it's okay to keep these type specified here so that they're are exported from the package and because they are only used for defaultTransactionTypeParser
There was a problem hiding this comment.
because then they'll be exported from web3-eth package
that make sense to not export it, with current code that you added.
|
Thanks @spacesailor24 |
| export const ERR_TX_POLLING_TIMEOUT = 426; | ||
| export const ERR_TX_RECEIPT_MISSING_OR_BLOCKHASH_NULL = 427; | ||
| export const ERR_TX_RECEIPT_MISSING_BLOCK_NUMBER = 428; | ||
| export const ERR_TX_INVALID_PROPERTIES_FOR_TYPE = 429; |
There was a problem hiding this comment.
Is it ok that we have the same code for two different errors?
ERR_TX_INVALID_PROPERTIES_FOR_TYPE & ERR_TX_LOCAL_WALLET_NOT_AVAILABLE
…order validation logic for each tx type
avkos
left a comment
avkos
left a comment
There was a problem hiding this comment.
LGTM.
BTW. Would be great to increase test coverage, thanks
@avkos This is the only line not covered by tests, and it's sorta irrelevant as it just a catch-all for any error that not a validation error 
|
4 participants
closes #6065