Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor formatters to JSON schemas and TypeScript band-aid solution #4859

Merged
merged 20 commits into from
Mar 29, 2022

Conversation

nazarhussain
Copy link
Contributor

Description

Please include a summary of the changes and be sure to follow our Contribution Guidelines.

Fixes #4778

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success.
  • I ran npm run test:cov and my test cases cover all the lines and branches of the added code.
  • I ran npm run build and tested dist/web3.min.js in a browser.
  • I have tested my code on the live network.
  • I have checked the Deploy Preview and it looks correct.
  • I have updated the CHANGELOG.md file in the root folder.

@nazarhussain nazarhussain marked this pull request as ready for review March 17, 2022 16:49
@nazarhussain nazarhussain self-assigned this Mar 17, 2022
@nazarhussain nazarhussain added the 4.x 4.0 related label Mar 17, 2022
Copy link
Contributor

@spacesailor24 spacesailor24 left a comment

Choose a reason for hiding this comment

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

I don't quite understand all that's going on in packages/web3-common/src/formatter.ts, but the use of the formatter seems better than what we had. Thank you Nazar

Only other feedback would be to pull any off the throw new Error('') into the respective errors.ts files

@nazarhussain
Copy link
Contributor Author

@spacesailor24 We had two challenges to solve in formatters with schema.

  1. To properly format the values
  2. To properly transform the output type

The value transformation is done through schema provided. The type formation is done through the source type of the data you pass. I tried to format the types based on schema, but that was very complex and didn't cover a lot edge cases, so we had to transform type from the source type. e.g.

Say we have data:

const data = {
     prop: 2
};

const schema = {type: 'object', properties: {prop: {eth: 'uint'}}};

In above the schema tells that prop is an unsigned integer value, and formatter will format this value into any format based on FMT_NUMBER enum you pass. But second problem to solve here is the type of the object data. Currently type of object is {prop: number}, if we format it to FMT_NUMBER.HEX then we have to convert the type to {prop: string} as well.

So in this formatting technique the source data type is very important. Any type attribute which is declared as number | bigint will be type formatted as FMT_NUMBER and any type attribute which is declared as Buffer | Uint8Array will be formatted baed on FMT_BYTES.

So to make it work properly, even if you know an attribute is hex string bytes, don't declare the type as string or HexString, declare it as Buffer, the rest will be handled by FMT_BYTES enum you pass, similarly for FMT_NUMBER.

@nazarhussain nazarhussain merged commit 6db0f4a into 4.x Mar 29, 2022
@nazarhussain nazarhussain deleted the nh/4778-formatter branch March 29, 2022 17:27
spacesailor24 added a commit that referenced this pull request Mar 31, 2022
… tests #4810 (#4875)

* WIP Web3Eth.sendTransaction tests

* Update use of ReceiptInfo and TransactionReceipt to ReceiptInfoFormatted

* Add optional data field to BaseTransaction

* Init sendTransaction test and fixture

* Remove old sendTransactionValidData

* Remove no longer needed imports

* Remove errornous sendTransaction test

* Init getGasPricing tests for sendTransaction

* Init wait_for_transaction_receipt.ts

* Init watch_transaction_for_confirmations.ts

* Import waitForTransactionReceipt and watchTransactionForConfirmations from utils/

* Init waitForTransactionReceipt and watchTransactionForConfirmations tests

* Fix promiEvent bug with sendSignedTransaction

* Use HexString for expectedTransactionHash instead of string

* Remove jest.useRealTimers

* Init sendSignedTransaction tests

* Update use of ReceiptInfo to ReceiptInfoFormatted for sendSignedTransaction

* Remove unused import ReceiptInfo

* Update SendSignedTransactionEvents to include only unique properties

* Add transactionHash to TransactionMissingReceiptOrBlockHashError

* Update packages/web3-eth/src/errors.ts

* Add effectiveGasPrice to sendSignedTransaction and sendTransaction fixtures

* Add expectedTransactionHash to watchTransactionForConfirmationsSpy tests

* Update names of files to match existing casings

* Merge with #4859

* Update import for ReceiptInfo

* Add effectiveGasPrice to transactionSchema

* Update SendTransactionEvents and SendSignedTransactionEvents types

* Add transaction receipt formatting

Co-authored-by: Alex <alex.luu@mail.utoronto.ca>
jdevcs pushed a commit that referenced this pull request Apr 12, 2022
* WIP Web3Eth.sendTransaction tests

* Update use of ReceiptInfo and TransactionReceipt to ReceiptInfoFormatted

* Add optional data field to BaseTransaction

* Init sendTransaction test and fixture

* Remove old sendTransactionValidData

* Remove no longer needed imports

* Remove errornous sendTransaction test

* Init getGasPricing tests for sendTransaction

* Init wait_for_transaction_receipt.ts

* Init watch_transaction_for_confirmations.ts

* Import waitForTransactionReceipt and watchTransactionForConfirmations from utils/

* Init waitForTransactionReceipt and watchTransactionForConfirmations tests

* Fix promiEvent bug with sendSignedTransaction

* Use HexString for expectedTransactionHash instead of string

* Remove jest.useRealTimers

* Init sendSignedTransaction tests

* Update use of ReceiptInfo to ReceiptInfoFormatted for sendSignedTransaction

* Remove unused import ReceiptInfo

* Update SendSignedTransactionEvents to include only unique properties

* Add transactionHash to TransactionMissingReceiptOrBlockHashError

* Update packages/web3-eth/src/errors.ts

* Possible solution without disabling linter for line

* Add effectiveGasPrice to sendSignedTransaction and sendTransaction fixtures

* Add expectedTransactionHash to watchTransactionForConfirmationsSpy tests

* Update names of files to match existing casings

* Merge with #4859

* Make first small change

* Add more changes

* Add effectiveGasPrice in tests

* Possible solution without disabling linter for line

* Make first small change

* Add more changes

* Add effectiveGasPrice in tests

* Add all changes

* Remove duplicate effectiveGasPrice entry

* Test commig signing

* Undo previous commit

* Update import for ReceiptInfo

* Add effectiveGasPrice to transactionSchema

* Update SendTransactionEvents and SendSignedTransactionEvents types

* Add transaction receipt formatting

Co-authored-by: Wyatt Barnes <me@wyatt.email>
Co-authored-by: Alex <alex.luu@mail.utoronto.ca>
nikoulai added a commit that referenced this pull request Apr 26, 2022
* WIP Web3Eth.sendTransaction tests

* Update use of ReceiptInfo and TransactionReceipt to ReceiptInfoFormatted

* Add optional data field to BaseTransaction

* Init sendTransaction test and fixture

* Remove old sendTransactionValidData

* Remove no longer needed imports

* Remove errornous sendTransaction test

* Init getGasPricing tests for sendTransaction

* Init wait_for_transaction_receipt.ts

* Init watch_transaction_for_confirmations.ts

* Import waitForTransactionReceipt and watchTransactionForConfirmations from utils/

* Init waitForTransactionReceipt and watchTransactionForConfirmations tests

* Fix promiEvent bug with sendSignedTransaction

* Use HexString for expectedTransactionHash instead of string

* Remove jest.useRealTimers

* Init sendSignedTransaction tests

* Update use of ReceiptInfo to ReceiptInfoFormatted for sendSignedTransaction

* Remove unused import ReceiptInfo

* Delete all .npmrc files

* Delete symbolic links too and update gitignore

* Remove Eth Transaction package

* Add license string in all ts files

* Update SendSignedTransactionEvents to include only unique properties

* Add transactionHash to TransactionMissingReceiptOrBlockHashError

* Update packages/web3-eth/src/errors.ts

* Fix small typo

* Possible solution without disabling linter for line

* Add effectiveGasPrice to sendSignedTransaction and sendTransaction fixtures

* Add expectedTransactionHash to watchTransactionForConfirmationsSpy tests

* Update names of files to match existing casings

* Merge with #4859

* Make first small change

* Add more changes

* Add effectiveGasPrice in tests

* Possible solution without disabling linter for line

* Make first small change

* Add more changes

* Add effectiveGasPrice in tests

* Add all changes

* Remove duplicate effectiveGasPrice entry

* Restore  .npmrc and .npmingnore

-Restore .npmrc in .gitignore
-Resore packages/web3-eth-abi

* Add rule

* Add missing license if files

* Update .eslintignore

* Add custom rule to lint script

* Add missing license if files

* Update lint rule

* Remove Eth Transaction package

* Add autofix licent eslint

* Add rule in lint:fix

* Refactor rule

* Add more comments

* Fix errors lint

* Update .eslintignore

* Update .eslintignore

* Fix order of comments

* Add missing license in files

* Test commig signing

* Undo previous commit

* Ignore .npmrc

* Update import for ReceiptInfo

* Add effectiveGasPrice to transactionSchema

* Update SendTransactionEvents and SendSignedTransactionEvents types

* Add transaction receipt formatting

* Refactor license eslint rule to use plugin instead of custom rule

* Hardcoded license message

* Update eslintignore

* Remove unecessary changes

* Add removed eslint rule

* Stop ignoring .npmrc

* Add license header in merged files

* Redelete unecessary tests

Co-authored-by: Wyatt Barnes <me@wyatt.email>
Co-authored-by: Alex <alex.luu@mail.utoronto.ca>
@nikoulai nikoulai restored the nh/4778-formatter branch May 11, 2022 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x 4.0 related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ETH Tx Utilities TODOs [1/5] - Refactor formatters to JSON schemas and TypeScript band-aid solution
3 participants