chore: use ethers for ethereum-storage #467

benjlevesque · 2021-03-26T23:42:15Z

Motivations

ethers is used everywhere in the codebase
web3 causes the node to crash if connection breaks
types and code readability

Additionally, this allows to remove hd-wallet-provider which limits the number of dependencies to maintain

coveralls · 2021-08-19T17:08:51Z

Coverage decreased (-0.3%) to 88.899% when pulling 43c4104 on eth-storage-ethers into 5fbc0b4 on master.

benjlevesque · 2021-12-02T14:29:12Z

packages/request-node/src/storageUtils.ts

+  const url = config.getStorageWeb3ProviderUrl();
+  if (url.match('^wss?://.+')) {
+    provider = new providers.WebSocketProvider(url);
+    // FIXME previous WS config


@alexandre-abrioux any idea what we should do here?

You can do this:

provider = new providers.Web3Provider( new Web3WsProvider(url, { // previous WS config }), )

; or, if we want to completely remove web3js dependencies, I can take care of it in another PR, for now just remove the if (wss) block or throw an "unsupported" error in the block, as ethers.js does not handle auto-reconnect, cf. ethers-io/ethers.js#1053

the point is to remove web3 completely yes

We should use this solution then: ethers-io/ethers.js#1053 (comment) , we just need to abstract the provider in it's own module.

benjlevesque · 2021-12-02T14:29:50Z

packages/integration-test/test/node-client.test.ts

@@ -494,6 +495,31 @@ describe('Request client using a request node', () => {
    const requests = await badRequestNetwork.fromTopic(myRandomTopic);
    expect(requests).toHaveLength(0);
  });
+
+  it('can create multiple requests in parallel', async () => {


I added this test because it seemed that the node wouldn't accespt multiple transactions at once.

benjlevesque · 2021-12-02T14:31:19Z

packages/request-node/src/storageUtils.ts

  } else {
-    provider = new HDWalletProvider(mnemonic, config.getStorageWeb3ProviderUrl());
+    provider = new providers.StaticJsonRpcProvider(url);


using a StaticJsonRpcProvider allows to reduce the number of calls (eth_chainId for instance)

I discovered it with ct. I should have told you so.

benjlevesque · 2021-12-02T14:44:41Z

packages/ethereum-storage/src/smart-contract-manager.ts

-      LENGTH_BYTES32_STRING,
+    const feesParametersAsBytes = utils.hexZeroPad(
+      utils.hexlify(feesParameters.contentSize),
+      LENGTH_BYTES32_STRING / 2,


because of the way hexZeroPad works

vrolland

Awesome PR ! Well done!

packages/ethereum-storage/src/ethereum-storage.ts

vrolland · 2021-12-03T09:38:47Z

packages/request-node/src/storageUtils.ts

  } else {
-    provider = new HDWalletProvider(mnemonic, config.getStorageWeb3ProviderUrl());
+    provider = new providers.StaticJsonRpcProvider(url);


I discovered it with ct. I should have told you so.

yomarion

Not easy to review but everything looks good to me, nice move !

yomarion · 2021-12-03T14:25:41Z

packages/ethereum-storage/src/smart-contract-manager.ts

-      // Keep the transaction hash for future needs
-      let transactionHash = '';
-      const transactionParameters = {
+    // This boolean is set to true once the ethereum metadata has been created and the promise has been resolved


yomarion · 2021-12-03T14:44:03Z

packages/ethereum-storage/test/smartcontract-manager.test.ts

-    expect(events[0].returnValues.feesParameters).toEqual(realSizeBytes32Hex);
+    expect(events[0].args.hash).toEqual(hashStr);
+    expect(events[0].args.hashSubmitter.toLowerCase()).toEqual(addressRequestHashSubmitter);
+    expect(events[0].args.feesParameters).toEqual(realSizeBytes32Hex);
  });

  // TODO since the migration to jest, this test fails.


Suggested change

// TODO since the migration to jest, this test fails.

Good one :)

yomarion · 2021-12-03T14:45:00Z

packages/ethereum-storage/test/smartcontract-manager.test.ts

+    jest
+      .spyOn(smartContractManager.ethereumBlocks.provider, 'getBlockNumber')
+      .mockImplementation(() => Promise.resolve(9));
+    // TODO ? smartContractManager.ethereumBlocks = new EthereumBlocks(mockEth, 1, 0, 0);


bertux

big good job 👍
I wondered if renaming eth as provider would make it more confusing later because it's more generic word but it's also the word used by etherjs so it's good for now.

alexandre-abrioux

Amazing refacto 💯

packages/ethereum-storage/src/config.ts

alexandre-abrioux · 2021-12-09T10:41:54Z

packages/ethereum-storage/test/ethereum-blocks.test.ts

@@ -103,34 +104,30 @@ const mockBlocksEthereum = [
  9807,
  9906,
 ];
+const provider = new providers.JsonRpcProvider('http://localhost:8545');


(Idea for later, to think about) We could do this to not depend on any external service for unit tests
(but this would reintroduce some web3 dependencies 😆)

import ganache from "ganache"; const provider = new ethers.providers.Web3Provider(ganache.provider());

MantisClone · 2022-12-05T22:15:09Z

Is this PR still relevant?

benjlevesque changed the base branch from master to bnjs March 27, 2021 00:40

benjlevesque force-pushed the eth-storage-ethers branch from 282f6df to 05815f8 Compare March 27, 2021 00:43

benjlevesque changed the title ~~eth storage ethers~~ chore: use ethers for ethereum-storage Mar 27, 2021

benjlevesque force-pushed the bnjs branch 2 times, most recently from 2cb737f to 8122ecc Compare March 29, 2021 15:46

Base automatically changed from bnjs to master March 29, 2021 16:03

benjlevesque force-pushed the eth-storage-ethers branch from 05815f8 to 441f044 Compare March 29, 2021 16:25

benjlevesque force-pushed the eth-storage-ethers branch from c8c8e33 to 6035e98 Compare August 19, 2021 14:25

benjlevesque added 10 commits September 22, 2021 16:23

chore: use ethers for ethereum-storage

9120681

retry mechanism & integration

b71a42a

smart-contract-manager tests

17e269a

wallet

7d300a4

layers

314c9f3

more web3 clean

def4881

ethers 5.4.1

6454888

fix tests

8ea690e

attempt runInBand

9bdf188

remove test helpers

ff6608f

benjlevesque force-pushed the eth-storage-ethers branch from 2a21082 to ff6608f Compare September 22, 2021 14:41

benjlevesque and others added 10 commits September 22, 2021 16:51

chai

9ec6af5

ts 4.4.3, ethers 5.4.7, test fixes

2f8b580

ts 4.4.3

5c50646

fix test

eca3d98

fix

f9b8e37

Merge branch 'master' into eth-storage-ethers

e81cb67

fix wrong merge

584a82d

try ethers 5.5.1

4806916

fix integration tests

1028ec8

chore: ws lib <= 7.4.6

33e6dee

github-actions bot requested review from bertux, kevindavee, KolevDarko and yomarion December 2, 2021 14:28

benjlevesque commented Dec 2, 2021

View reviewed changes

revert gas price change

4710326

benjlevesque commented Dec 2, 2021

View reviewed changes

vrolland approved these changes Dec 3, 2021

View reviewed changes

yomarion approved these changes Dec 3, 2021

View reviewed changes

bertux approved these changes Dec 7, 2021

View reviewed changes

benjlevesque added 2 commits December 7, 2021 11:35

comments

06808ab

Merge remote-tracking branch 'origin/master' into eth-storage-ethers

3bdc5ac

benjlevesque requested a review from alexandre-abrioux December 8, 2021 14:01

Merge branch 'master' into eth-storage-ethers

ef05632

alexandre-abrioux approved these changes Dec 9, 2021

View reviewed changes

benjlevesque added 3 commits December 9, 2021 13:29

remove comment

0041d4e

revert config change & fix type

6f63768

Merge branch 'master' into eth-storage-ethers

43c4104

benjlevesque enabled auto-merge (squash) December 9, 2021 11:40

benjlevesque and others added 2 commits December 9, 2021 16:57

increase timeout

b391c45

Merge branch 'master' into eth-storage-ethers

cd0a4c9

KolevDarko approved these changes Dec 15, 2021

View reviewed changes

benjlevesque removed the request for review from kevindavee April 29, 2022 13:35

benjlevesque marked this pull request as draft May 19, 2022 07:52

auto-merge was automatically disabled May 19, 2022 07:52
Pull request was converted to draft

benjlevesque closed this Dec 6, 2022

benjlevesque deleted the eth-storage-ethers branch December 6, 2022 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: use ethers for ethereum-storage #467

chore: use ethers for ethereum-storage #467

benjlevesque commented Mar 26, 2021 •

edited

Loading

coveralls commented Aug 19, 2021 •

edited

Loading

benjlevesque Dec 2, 2021

alexandre-abrioux Dec 2, 2021 •

edited

Loading

benjlevesque Dec 2, 2021

alexandre-abrioux Dec 2, 2021

benjlevesque Dec 2, 2021

benjlevesque Dec 2, 2021

vrolland Dec 3, 2021

benjlevesque Dec 2, 2021

vrolland left a comment

vrolland Dec 3, 2021

yomarion left a comment

yomarion Dec 3, 2021

yomarion Dec 3, 2021

yomarion Dec 3, 2021

bertux left a comment

alexandre-abrioux left a comment

alexandre-abrioux Dec 9, 2021 •

edited

Loading

MantisClone commented Dec 5, 2022

chore: use ethers for ethereum-storage #467

chore: use ethers for ethereum-storage #467

Conversation

benjlevesque commented Mar 26, 2021 • edited Loading

coveralls commented Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

alexandre-abrioux Dec 2, 2021 • edited Loading

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

vrolland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yomarion 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

bertux left a comment

Choose a reason for hiding this comment

alexandre-abrioux left a comment

Choose a reason for hiding this comment

alexandre-abrioux Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

MantisClone commented Dec 5, 2022

benjlevesque commented Mar 26, 2021 •

edited

Loading

coveralls commented Aug 19, 2021 •

edited

Loading

alexandre-abrioux Dec 2, 2021 •

edited

Loading

alexandre-abrioux Dec 9, 2021 •

edited

Loading