-
Notifications
You must be signed in to change notification settings - Fork 5.1k
feat: add custom transaction schema to formatTransaction #7227
feat: add custom transaction schema to formatTransaction #7227
Conversation
8abb6e7 to
fd80328
Compare
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.
@nicolasbrugneaux Thanks for your contribution, there are some build issues that need fix.
01ca836 to
596224c
Compare
I'm having issues running it locally but I think I fixed it all. Any chance we can re-run ci? |
|
some of tests needs update, including unit test: |
e0ec4be to
796660d
Compare
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.
LGTM. But still, some tests fail. Will be great to fix them cause I see some types incompatible.
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 @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,
90d9102 to
e335b4e
Compare
|
@nicolasbrugneaux great effort, for this feature. I suggested few changes. Thanks for your contributions |
3ce8752 to
1f195e0
Compare
1f195e0 to
125bf12
Compare
|
@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 |
4a0519d to
0348f44
Compare
|
Hey @nicolasbrugneaux , https://github.com/web3/web3.js/actions/runs/10886942923/job/30214851919?pr=7227 the build is currently failing |
|
Fixed the build, builds locally at least 😅 |
|
almost there 😅 |
|
I noticed the interface for |
|
Thank you!! |
|
hey there @luu-alex since you kindly merged this I wanted to bring your attention to a follow-up issue I encountered: |
Greetings 👋
Description
This PR aims to solidify the Plugin experience by allowing a
customTransactionSchemato be used byformatTransaction. This seems to have be thought of before as theformatTransactionalready accepted a custom schema in its parameters but it seems it wasn't used anywhere yet. This PR aims solve that.Type of change
Checklist:
npm run lintwith success and extended the tests and types if necessary.npm run test:unitwith success.npm run test:coverageand my test cases cover all the lines and branches of the added code.npm run buildand testeddist/web3.min.jsin a browser.CHANGELOG.mdfile in the root folder.