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

feat: add custom transaction schema to formatTransaction #7227

Merged
merged 19 commits into from
Sep 17, 2024

Conversation

nicolasbrugneaux
Copy link
Contributor

@nicolasbrugneaux nicolasbrugneaux commented Aug 30, 2024

Greetings 👋

Description

This PR aims to solidify the Plugin experience by allowing a customTransactionSchema to be used by formatTransaction. This seems to have be thought of before as the formatTransaction already accepted a custom schema in its parameters but it seems it wasn't used anywhere yet. This PR aims solve that.

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 lint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success.
  • I ran npm run test:coverage 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.
  • I have linked Issue(s) with this PR in "Linked Issues" menu.

@nicolasbrugneaux nicolasbrugneaux force-pushed the nicolasbrugneaux/customTransactionSchema branch from 8abb6e7 to fd80328 Compare August 30, 2024 15:48
Copy link
Contributor

@jdevcs jdevcs left a comment

Choose a reason for hiding this comment

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

@nicolasbrugneaux Thanks for your contribution, there are some build issues that need fix.

@nicolasbrugneaux nicolasbrugneaux force-pushed the nicolasbrugneaux/customTransactionSchema branch from 01ca836 to 596224c Compare September 2, 2024 13:25
@nicolasbrugneaux
Copy link
Contributor Author

@nicolasbrugneaux Thanks for your contribution, there are some build issues that need fix.

I'm having issues running it locally but I think I fixed it all. Any chance we can re-run ci?

@jdevcs
Copy link
Contributor

jdevcs commented Sep 4, 2024

some of tests needs update, including unit test:

web3-eth:  FAIL  test/unit/format_transaction.test.ts
web3-eth:   ● Test suite failed to run
web3-eth:     test/unit/format_transaction.test.ts:123:52 - error TS2339: Property 'feeCurrency' does not exist on type '{ from?: string | undefined; to?: string | null | undefined; value?: bigint | undefined; accessList?: { readonly address?: string | undefined; readonly storageKeys?: string[] | undefined; }[] | undefined; ... 17 more ...; s?: string | undefined; }'.
web3-eth:     123   expect(formatTransaction(customFieldTransaction).feeCurrency).toBeUndefined();
web3-eth:                                                            ~~~~~~~~~~~
web3-eth:     test/unit/format_transaction.test.ts:130:7 - error TS2322: Type '{ properties: { feeCurrency: string; from: { format: string; }; to: { oneOf: ({ format: string; type?: undefined; } | { type: string; format?: undefined; })[]; }; value: { format: string; }; gas: { format: string; }; ... 17 more ...; s: { ...; }; }; type: string; }' is not assignable to type 'ValidationSchemaInput | { type: string; properties: { from: { format: string; }; to: { oneOf: ({ format: string; type?: undefined; } | { type: string; format?: undefined; })[]; }; ... 19 more ...; s: { ...; }; }; } | undefined'.
web3-eth:       Types of property 'properties' are incompatible.
web3-eth:         Type '{ feeCurrency: string; from: { format: string; }; to: { oneOf: ({ format: string; type?: undefined; } | { type: string; format?: undefined; })[]; }; value: { format: string; }; gas: { format: string; }; gasPrice: { format: string; }; ... 16 more ...; s: { ...; }; }' is not assignable to type '{ from: { format: string; }; to: { oneOf: ({ format: string; type?: undefined; } | { type: string; format?: undefined; })[]; }; value: { format: string; }; gas: { format: string; }; gasPrice: { format: string; }; effectiveGasPrice: { ...; }; ... 15 more ...; s: { ...; }; }'.
web3-eth:           Object literal may only specify known properties, and 'feeCurrency' does not exist in type '{ from: { format: string; }; to: { oneOf: ({ format: string; type?: undefined; } | { type: string; format?: undefined; })[]; }; value: { format: string; }; gas: { format: string; }; gasPrice: { format: string; }; effectiveGasPrice: { ...; }; ... 15 more ...; s: { ...; }; }'.
web3-eth:     130       feeCurrency: 'address',
web3-eth:               ~~~~~~~~~~~~~~~~~~~~~~
web3-eth:     test/unit/format_transaction.test.ts:133:7 - error TS2339: Property 'feeCurrency' does not exist on type '{ from?: string | undefined; to?: string | null | undefined; value?: bigint | undefined; accessList?: { readonly address?: string | undefined; readonly storageKeys?: string[] | undefined; }[] | undefined; ... 17 more ...; s?: string | undefined; }'.
web3-eth:     133    }).feeCurrency,
web3-eth:               ~~~~~~~~

packages/web3-eth/CHANGELOG.md Outdated Show resolved Hide resolved
packages/web3-eth/src/rpc_method_wrappers.ts Outdated Show resolved Hide resolved
packages/web3-eth/src/utils/decode_signed_transaction.ts Outdated Show resolved Hide resolved
@nicolasbrugneaux nicolasbrugneaux force-pushed the nicolasbrugneaux/customTransactionSchema branch from e0ec4be to 796660d Compare September 6, 2024 14:02
Copy link
Contributor

@avkos avkos left a comment

Choose a reason for hiding this comment

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

LGTM. But still, some tests fail. Will be great to fix them cause I see some types incompatible.

Copy link
Contributor

@Muhammad-Altabba Muhammad-Altabba left a comment

Choose a reason for hiding this comment

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

Thanks @nicolasbrugneaux,
Could you please revert the code formatting done in this MR so we can easily review it? (We are working on a better formatting for the code which will be in a separate MR).
Also please fix the tests so we can look at your MR deeply.
Thanks,

@nicolasbrugneaux nicolasbrugneaux force-pushed the nicolasbrugneaux/customTransactionSchema branch 5 times, most recently from 90d9102 to e335b4e Compare September 11, 2024 07:45
@luu-alex
Copy link
Contributor

@nicolasbrugneaux thanks for your contribution, the tests look like they are passing! @jdevcs if everything looks good we can merge this, and i can include this for release next week.

@jdevcs
Copy link
Contributor

jdevcs commented Sep 13, 2024

@nicolasbrugneaux great effort, for this feature. I suggested few changes. Thanks for your contributions

@nicolasbrugneaux nicolasbrugneaux force-pushed the nicolasbrugneaux/customTransactionSchema branch from 3ce8752 to 1f195e0 Compare September 16, 2024 14:52
@nicolasbrugneaux nicolasbrugneaux force-pushed the nicolasbrugneaux/customTransactionSchema branch from 1f195e0 to 125bf12 Compare September 16, 2024 14:54
@nicolasbrugneaux
Copy link
Contributor Author

@jdevcs thanks for the thourough review. I should have addressed all feedback. Let me know if you were thinking of doing it differently.

Side note: I also refactored the type of config.customTransactionSchema to be more generic and thus not have to import {CustomTransactionSchema} from 'web3-eth' into web3-core. Let me know if you think there's a better place for that new type.

@nicolasbrugneaux nicolasbrugneaux force-pushed the nicolasbrugneaux/customTransactionSchema branch from 4a0519d to 0348f44 Compare September 16, 2024 15:24
@luu-alex
Copy link
Contributor

Hey @nicolasbrugneaux , https:/web3/web3.js/actions/runs/10886942923/job/30214851919?pr=7227 the build is currently failing

@nicolasbrugneaux
Copy link
Contributor Author

Fixed the build, builds locally at least 😅

@luu-alex
Copy link
Contributor

almost there 😅
web3-core: FAIL test/unit/web3_context.test.ts
web3-core: ● Test suite failed to run
web3-core: test/unit/web3_context.test.ts:127:37 - error TS2339: Property 'config' does not exist on type 'Web3RequestManager'.
web3-core: 127 expect(newContext.requestManager.config.defaultHardfork).toEqual(
web3-core:

@nicolasbrugneaux
Copy link
Contributor Author

I noticed the interface for config.customTransactionSchema was incorrect so I fixed that as well as the failing test above.

@jdevcs jdevcs requested a review from avkos September 17, 2024 15:38
@luu-alex luu-alex merged commit a21078b into web3:4.x Sep 17, 2024
16 of 18 checks passed
@luu-alex luu-alex mentioned this pull request Sep 17, 2024
17 tasks
@nicolasbrugneaux nicolasbrugneaux deleted the nicolasbrugneaux/customTransactionSchema branch September 18, 2024 07:27
@nicolasbrugneaux
Copy link
Contributor Author

Thank you!!

@nicolasbrugneaux
Copy link
Contributor Author

hey there @luu-alex since you kindly merged this I wanted to bring your attention to a follow-up issue I encountered:

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

Successfully merging this pull request may close these issues.

6 participants