-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Switch from web3.js to ethers.js #15760
base: develop
Are you sure you want to change the base?
Conversation
Thank you for your contribution to the Solidity compiler! A team member will follow up shortly. If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother. If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix. |
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.
Overall looks good, but I think we should address some questions/suggestions before.
@@ -181,21 +181,19 @@ arguments ``from``, ``to`` and ``amount``, which makes it possible to track | |||
transactions. | |||
|
|||
To listen for this event, you could use the following | |||
JavaScript code, which uses `web3.js <https://github.com/web3/web3.js/>`_ to create the ``Coin`` contract object, | |||
JavaScript code, which uses `ethers.js <https://github.com/ethers-io/ethers.js>`_ to create the ``coin`` contract object, |
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.
JavaScript code, which uses `ethers.js <https://github.com/ethers-io/ethers.js>`_ to create the ``coin`` contract object, | |
JavaScript code, which uses `ethers.js <https://github.com/ethers-io/ethers.js>`_ to create the ``Coin`` contract object, |
Here Coin
refers to the contract name, not the variable name in the example.
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.
Here
Coin
refers to the contract name, not the variable name in the example.
Got it, thanks for clarifying!
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.
Also, this is failing because there are trailing whitespaces that needed to be removed in lines 191
and 194
Fixed |
@codersjj can you rebase and squash the commits, please? |
Sure! I've rebased and squashed the commits. However, I noticed an extra merge commit was generated. Is this acceptable, or should I clean it up? Let me know! 😊 |
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 for the contribution! The code looks good overall.
However, the PR does not seem to be addressing #15759 completely (despite the Resolves: #15759
in the description). There are a few more web3.js snippets in the docs and also some mentions of the library.
I don't think it really makes sense to convert just one example and leave everything else as is. Having one in some places and the other in others might be even worse than just leaving it as is.
const senderBalance = await coin.balances(from); | ||
const receiverBalance = await coin.balances(to); | ||
console.log(`Balances now:\nSender: ${ethers.formatUnits(senderBalance, 18)}\nReceiver: ${ethers.formatUnits(receiverBalance, 18)}`); |
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.
The original formatting here was more readable. No reason to remove it.
console.log(`Balances now:\nSender: ${ethers.formatUnits(senderBalance, 18)}\nReceiver: ${ethers.formatUnits(receiverBalance, 18)}`); | |
console.log( | |
`Balances now:\n` + | |
`Sender: ${ethers.formatUnits(senderBalance, 18)}\n` + | |
`Receiver: ${ethers.formatUnits(receiverBalance, 18)}` | |
); |
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.
Note that this is not the only place where we're using web3.js. We have a few more examples/mentions:
We should probably also mention deprecation in the "Resources" section.
Rebasing would have removed the merge commit. I think you must have merged |
Thanks for pointing that out—I completely agree! 🙌 You're right that consistency is key here. I'll go through the docs to find any remaining web3.js snippets/mentions and update them to use ethers.js instead. Appreciate the thorough review, and please feel free to flag any other instances I might miss! |
f131eeb
to
1230992
Compare
Update introduction-to-smart-contracts.rst Update Code Examples from Using Web3.js to Ethers.js Update docs/introduction-to-smart-contracts.rst Co-authored-by: matheusaaguiar <95899911+matheusaaguiar@users.noreply.github.com> Update docs/introduction-to-smart-contracts.rst Co-authored-by: matheusaaguiar <95899911+matheusaaguiar@users.noreply.github.com> remove trailing whitespaces Update introduction-to-smart-contracts.rst Update Code Examples from Using Web3.js to Ethers.js Update docs/introduction-to-smart-contracts.rst Co-authored-by: Kamil Śliwak <cameel2@gmail.com> docs: switch from web3.js to ethers.js Update Code Examples from Using Web3.js to Ethers.js Update docs/introduction-to-smart-contracts.rst Co-authored-by: matheusaaguiar <95899911+matheusaaguiar@users.noreply.github.com> Update docs/introduction-to-smart-contracts.rst Co-authored-by: matheusaaguiar <95899911+matheusaaguiar@users.noreply.github.com> remove trailing whitespaces Update introduction-to-smart-contracts.rst Update Code Examples from Using Web3.js to Ethers.js Update docs/introduction-to-smart-contracts.rst Co-authored-by: Kamil Śliwak <cameel2@gmail.com> docs: switch from web3.js to ethers.js
Update Code Examples from Using Web3.js to Ethers.js
Resolves: #15759