Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

web3js 4.x RC upgrade #6013

Closed

Conversation

nikoulai
Copy link

@nikoulai nikoulai commented May 3, 2023

web3js 4.x

@jdevcs
Copy link

jdevcs commented May 3, 2023

Thanks @nikoulai .
@haltman-at @eggplantzzz need your feedback in this PR ,
and also could you trigger tests in CI. Thanks

Comment on lines -164 to +168
assert(error.message.includes("intrinsic gas too low"));
// assert(error.innerError.message.includes("intrinsic gas too low"));

Choose a reason for hiding this comment

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

Commented out line

Comment on lines -268 to +277
it("overrides 50 blocks err / return a usable instance", async function () {
//todo web3js-migration fix test
it.skip("overrides 50 blocks err / return a usable instance", async function () {

Choose a reason for hiding this comment

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

Unhandled TODO

Comment on lines +315 to +317
//todo web3js-migration fix test
it.skip(
"overrides gateway tx propagation delay err / return a usable instance",

Choose a reason for hiding this comment

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

Unhandled TODO

Comment on lines -151 to +152
it("validates method arguments for .sends", async function () {
//todo web3js-migration remove skip
it.skip("validates method arguments for .sends", async function () {

Choose a reason for hiding this comment

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

Unhandled TODO

this.removeListener("error", handlers.error);
this.off("error", handlers.error);

Choose a reason for hiding this comment

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

Shouldn't removeListener work here? What was the issue?

Comment on lines -20 to +19
return web3Utils.toBN(val);
return new BN(val);

Choose a reason for hiding this comment

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

FYI to Truffle team, we no longer use bignumber.js or bn.js, just BigInts

Comment on lines -37 to +40
return web3Utils.isBN(val) || web3Utils.isBigNumber(val);
//todo web3js-migration check if this is sufficient
return BN.isBN(val);
// || web3Utils.isBigNumber(val);

Choose a reason for hiding this comment

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

Unhandled TODO

addresses.push(address);
addresses.push(address as string);

Choose a reason for hiding this comment

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

What was the type of address? It should be a string by default

Comment on lines +155 to +156
const ZERO_NODE =
"0x0000000000000000000000000000000000000000000000000000000000000000";

Choose a reason for hiding this comment

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

Should remove if "0x0" suffices

@@ -327,6 +327,7 @@ class Deployment {
eventArgs.error = err.error || err;
const message = await self.emit("deployment:failed", eventArgs);
self.close();
console.warn(message);

Choose a reason for hiding this comment

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

Should remove

Comment on lines +306 to +316
console.log(libReceipt.blockNumber, startBlock);
console.log(
exampleReceipt.blockNumber,
libReceipt.blockNumber,
startBlock
);
// The first confirmation is the block that accepts the tx. Then we wait two more.
// Then Example is deployed in the consequent block.
assert(libReceipt.blockNumber === startBlock + 1);
assert(exampleReceipt.blockNumber === libReceipt.blockNumber + 3);
assert(libReceipt.blockNumber === startBlock + BigInt(1));
//todo web3js-migration this fails
// assert(exampleReceipt.blockNumber === libReceipt.blockNumber + BigInt(3));

Choose a reason for hiding this comment

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

Unhandled TODO

Copy link

Choose a reason for hiding this comment

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

@nikoulai Did you add all this? Where is it from, what's it for?

Same with the files:

  • packages/encoder/test/initialization.test.js
  • packages/encoder/test/integer.test.js
  • packages/encoder/test/udvt.test.js

And these lib/ files shouldn't be included?

  • packages/fetch-and-compile/lib/debug.js
  • packages/fetch-and-compile/lib/fetch.js
  • packages/fetch-and-compile/lib/index.js
  • packages/fetch-and-compile/lib/multiple.js
  • packages/fetch-and-compile/lib/recognizer.js
  • packages/fetch-and-compile/lib/types.js
  • packages/fetch-and-compile/lib/utils.js

@cds-amal
Copy link
Member

cds-amal commented May 4, 2023

Note: @nazarhussain was (is?) working this issue and there might be some learnings in his PR #5393.

@nazarhussain
Copy link

@cds-amal @nikoulai Was holding it off to get fixed the identified issues in web3-4x first, publish it into a release, and then continue with this upgrade process.

So that you know, I identified the following list of issues during this upgrade process. This is the reason my PR taking long time to shape up.

web3/web3.js#5287
web3/web3.js#5288
web3/web3.js#5232
web3/web3.js#5208
web3/web3.js#5207
web3/web3.js#5201
web3/web3.js#5090
web3/web3.js#5203
web3/web3.js#5196
web3/web3.js#5189
web3/web3.js#5171
web3/web3.js#5170
web3/web3.js#5169
web3/web3.js#5127
web3/web3.js#5126
web3/web3.js#5125
web3/web3.js#5319
web3/web3.js#5320
web3/web3.js#5321
web3/web3.js#5712
web3/web3.js#5714
web3/web3.js#5717

@nikoulai nikoulai closed this May 9, 2023
@nazarhussain
Copy link

For clarity, @nikoulai and I discussed and agreed to make the #5393 as a base for this work. And @nikoulai will continue on that PR further.

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

Successfully merging this pull request may close these issues.

None yet

5 participants