Ethers.js Update #303

b-pmcg · 2021-11-15T22:57:11Z

No description provided.

b-pmcg · 2021-11-15T23:03:57Z

This is ready to begin the review process. There are some comments and console.logs that I will need to clean up, as well as fixing the package imports. I changed these to read from my local version so I didn't have to keep publishing alphas repeatedly.

I will try to add context in the comments to some of the changes to make it easier to review.

b-pmcg · 2021-11-16T19:40:16Z

packages/dai-plugin-governance/src/ApproveLinkTransaction.js

+    const [topic] = this._contract.interface.encodeFilterTopics(
+      'LinkConfirmed',
+      []


Good example of an ethers built in function that simplifies things a little bit from before.

b-pmcg · 2021-11-16T19:43:16Z

packages/dai-plugin-governance/src/ChiefService.js

@@ -223,13 +223,13 @@ export default class ChiefService extends LocalService {
  getNumDeposits(address) {
    return this._chiefContract()
      .deposits(address)
-      .then(MKR.wei);
+      .then(n => MKR.wei(n._hex));


Since all our contracts are now created with ethers 5, the returned number value is of BN.js bignumber type (not bignumber.js). Passing a BN bignumber into currency does not work, so instead we now have to pass it's hex value (passing .toString() would also work, but I figure the hex value is less prone to a conversion error).

This is the first of many examples of converting an ethers returned BN type into a hex for currency to consume.

b-pmcg · 2021-11-16T19:46:20Z

packages/dai-plugin-liquidations/src/LiquidationService.ts

-  return '0x' + Buffer.from(str).toString('hex');
-};
+export function stringToBytes(str) {
+  return utils.formatBytes32String(str);


Ethers is more strict about types, in some places where we used to pass a function signature as 0x4554482d41, we now have to pass the full 32 bytes, eg. 0x4554482d41000000000000000000000000000000000000000000000000000000. Fortunately ethers utils has a built in function for that.

b-pmcg · 2021-11-16T19:52:03Z

packages/dai-plugin-scd/src/tokens/Erc20Token.js

@@ -59,7 +61,8 @@ export default class Erc20Token {

  approveUnlimited(spender, options = {}) {
    if (!spender) spender = this._web3.currentAddress();
-    return this._contract.approve(spender, -1, {
+    return this._contract.approve(spender, ethers.BigNumber.from(UINT256_MAX), {


can no longer approve -1 as a shortcut for unlimited in ethers 5.

i tried to find documentation for this but i can not

@rafinskipg in web3.js it seems a negative number is interpreted as int256. Maybe ethers v3 accepted this as well? But you can't send a negative number in EVM according to this.

b-pmcg · 2021-11-16T19:53:09Z

packages/dai/src/eth/TransactionManager.js

@@ -196,13 +196,16 @@ export default class TransactionManager extends PublicService {

  async _getGasLimit(options, contract, method, args) {
    let transaction = {};
-    let data = contract.interface.functions[method](...args).data;
+    let data = contract.interface.encodeFunctionData(method, args);


The old way to get contract function data was deprecated, now we use encodeFunctionData

codecov · 2021-11-16T19:53:38Z

Codecov Report

Merging #303 (32acd54) into dev (2af8f5c) will decrease coverage by 0.01%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##              dev     #303      +/-   ##
==========================================
- Coverage   86.13%   86.11%   -0.02%     
==========================================
  Files         139      139              
  Lines        6310     6339      +29     
  Branches     1247     1258      +11     
==========================================
+ Hits         5435     5459      +24     
- Misses        870      875       +5     
  Partials        5        5

Impacted Files	Coverage Δ
packages/dai-plugin-scd/src/tokens/Erc20Token.js	`0.00% <0.00%> (ø)`
packages/dai-plugin-scd/src/tokens/PethToken.js	`0.00% <0.00%> (ø)`
packages/dai/src/eth/tokens/PethToken.ts	`66.66% <0.00%> (ø)`
packages/dai-plugin-migrations/src/utils.ts	`39.39% <50.00%> (+1.89%)`	⬆️
packages/dai/src/eth/Web3Service.ts	`83.21% <66.66%> (-0.38%)`	⬇️
packages/dai/src/eth/web3/ShimEthersSigner.js	`90.00% <66.66%> (-4.12%)`	⬇️
packages/dai-plugin-mcd/src/utils.ts	`89.47% <75.00%> (-1.27%)`	⬇️
...ai-plugin-governance/src/ApproveLinkTransaction.js	`100.00% <100.00%> (ø)`
packages/dai-plugin-governance/src/ChiefService.js	`85.60% <100.00%> (+0.22%)`	⬆️
packages/dai-plugin-governance/src/EsmService.ts	`94.54% <100.00%> (ø)`
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2af8f5c...32acd54. Read the comment docs.

b-pmcg · 2021-11-16T19:58:50Z

packages/dai/src/eth/smartContract/wrapContract.js

+          for (const fnKey in contract.interface.functions) {
+            // Match the function override with key that has the same number of inputs
+            if (
+              contract.interface.functions[fnKey].name === key &&
+              contract.interface.functions[fnKey].inputs.length ===
+                functionInputsLength
+            ) {
+              key = fnKey;
+            }
+          }
+          return txManager.sendContractCall(contract, key, args, name);
+        };


We now have to be more specific with function overloads in ethers 5, so we have to specify the exact function signature, rather than the function name. Here we determine the correct key by matching the number of inputs from the interface with the number of args passed in (minus the business object at the end).

adamgoth

Left a couple questions but looks good to me otherwise. Nice work. Looks like it was a fun one 😛

adamgoth · 2021-11-17T13:50:19Z

packages/dai-plugin-mcd/src/EventHistory.js

+  // const MCD_JOIN_SAI = cdpManager
+  //   .get('smartContract')
+  //   .getContractAddress('MCD_JOIN_SAI');


Do we want to leave this commented out?

adamgoth · 2021-11-17T13:51:46Z

packages/dai-plugin-mcd/src/EventHistory.js

+              /*
+              FIXME: ethers 5 doesn't support passing an array of addresses into getLogs
+              as we move further away from SCD, join Sai events are less likely, but
+              we should eventually re-add this functionality by duplicating this call for MCD_JOIN_SAI.
+              */
+              // address: [MCD_JOIN_DAI, MCD_JOIN_SAI],
+              address: MCD_JOIN_DAI,


Same question here. How do you think we should address this?

I left it there commented out as a reminder, but I don't know if we should prioritize it at the moment. Given that nobody uses the event history in the mcd plugin anymore since Oasis moved away from it, and given that it only affects parsing SCD events, I'm willing to release this version without it. If anyone feels differently let me know.

The fix would be to duplicate the same function using MCD_JOIN_SAI as the address, merge & sort the results.

adamgoth · 2021-11-17T13:52:17Z

packages/dai-plugin-mcd/src/utils.ts

 export function stringToBytes(str) {
  assert(!!str, 'argument is falsy');
  assert(typeof str === 'string', 'argument is not a string');
-  return '0x' + Buffer.from(str).toString('hex');
+  return ethersUtils.formatBytes32String(str);


adamgoth · 2021-11-17T13:54:49Z

scripts/run-testchain.sh

@@ -3,5 +3,5 @@ set -e

 CWD="${0%/*}"

-$CWD/set-polling-interval.sh


Not sure if this should be left commented out either. Flagging to review as well.

it was making a change to a file in ethers that no longer exists, but I'll double check if I can change the path to the file it was looking for.

rafinskipg

i trust you , and i trust the test suite

- @makerdao/[email protected] - @makerdao/[email protected] - @makerdao/[email protected] - @makerdao/[email protected] - @makerdao/[email protected] - @makerdao/[email protected] - @makerdao/[email protected] - @makerdao/[email protected]

b-pmcg added 7 commits November 13, 2021 13:19

update ethers to 5 without removing web3

06a1840

fix mcd and governance tests

8679eea

fix some scd tests

f66dc9e

update alphas

872100b

fix remaining scd tests

97fcea6

fix migration tests

5b6baef

fix liquidations tests

3200731

b-pmcg changed the title ~~Ethersjs no provider~~ Ethers.js Update Nov 15, 2021

b-pmcg requested review from rafinskipg, tyler17 and adamgoth November 15, 2021 23:01

b-pmcg marked this pull request as draft November 15, 2021 23:01

b-pmcg commented Nov 16, 2021

View reviewed changes

clean up imports

5d77617

b-pmcg force-pushed the ethersjs-no-provider branch from e529a1a to 5d77617 Compare November 16, 2021 20:01

b-pmcg marked this pull request as ready for review November 16, 2021 20:02

adamgoth approved these changes Nov 17, 2021

View reviewed changes

adamgoth reviewed Nov 17, 2021

View reviewed changes

rafinskipg approved these changes Nov 17, 2021

View reviewed changes

b-pmcg added 4 commits November 17, 2021 17:19

bump versions

fc3a8ac

Publish

1ed7521

- @makerdao/[email protected] - @makerdao/[email protected] - @makerdao/[email protected] - @makerdao/[email protected] - @makerdao/[email protected] - @makerdao/[email protected] - @makerdao/[email protected] - @makerdao/[email protected]

bump test-helpers version

048d2b9

fix merge conflicts with dev

32acd54

b-pmcg merged commit 76f7ddc into dev Nov 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ethers.js Update #303

Ethers.js Update #303

b-pmcg commented Nov 15, 2021

b-pmcg commented Nov 15, 2021

b-pmcg Nov 16, 2021

b-pmcg Nov 16, 2021

b-pmcg Nov 16, 2021 •

edited

Loading

b-pmcg Nov 16, 2021

rafinskipg Nov 17, 2021

b-pmcg Nov 17, 2021 •

edited

Loading

b-pmcg Nov 16, 2021

codecov bot commented Nov 16, 2021 •

edited

Loading

b-pmcg Nov 16, 2021

adamgoth left a comment

adamgoth Nov 17, 2021

adamgoth Nov 17, 2021

b-pmcg Nov 17, 2021

adamgoth Nov 17, 2021

adamgoth Nov 17, 2021

b-pmcg Nov 17, 2021

rafinskipg left a comment

		@@ -3,5 +3,5 @@ set -e

		CWD="${0%/*}"

		$CWD/set-polling-interval.sh

Ethers.js Update #303

Ethers.js Update #303

Conversation

b-pmcg commented Nov 15, 2021

b-pmcg commented Nov 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-pmcg Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-pmcg Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 16, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

adamgoth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafinskipg left a comment

Choose a reason for hiding this comment

b-pmcg Nov 16, 2021 •

edited

Loading

b-pmcg Nov 17, 2021 •

edited

Loading

codecov bot commented Nov 16, 2021 •

edited

Loading