-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Web3 4x Upgrade #5393
base: develop
Are you sure you want to change the base?
Web3 4x Upgrade #5393
Conversation
Ooh nice! Thanks, @nazarhussain 🙇 |
Yeah thanks @nazarhussain! Just a note, I saw that you skipped the Vyper compiler tests. If you want to be able to run those successfully you will have to install the Vyper compiler locally. https://vyper.readthedocs.io/en/stable/installing-vyper.html I think I had to use the instructions using pip. |
@eggplantzzz There are few tests which I am still working on. So why I kept the PR in draft mode for now. As soon all tests passes I will mark it ready. |
@nazarhussain the yarncheck failure indicates |
@cds-amal After running the
|
@nazarhussain it looks like you may have built the $ grep localhost yarn.lock | head -n 5
resolved "http://localhost:4873/@ethereumjs%2fcommon/-/common-2.6.5.tgz#0a75a22a046272579d91919cb12d84f2756e8d30"
resolved "http://localhost:4873/@ethereumjs%2ftx/-/tx-3.5.2.tgz#197b9b6299582ad84f9527ca961466fce2296c1c"
resolved "http://localhost:4873/@ethersproject%2fabi/-/abi-5.6.4.tgz#f6e01b6ed391a505932698ecc0d9e7a99ee60362"
resolved "http://localhost:4873/@noble%2fsecp256k1/-/secp256k1-1.6.3.tgz#7eed12d9f4404b416999d0c87686836c4c5c9b94"
resolved "http://localhost:4873/@scure%2fbip39/-/bip39-1.1.0.tgz#92f11d095bae025f166bef3defcc5bf4945d419a"
$ git log -S"http://localhost:4873"
Thu Aug 4 23:01:18 2022 +0200 29671cc03 (HEAD -> nh/web3-4x-upgrade) :art: Fix the issues raised after merging base branch [Nazar Hussain]
Thu Aug 4 15:34:12 2022 +0200 8c2d7c34e :arrow_up: Update dependenceies [Nazar Hussain]% |
@cds-amal Yes I am using local NPM registry because these packages are not yet published. I will update yarn.lock soon after the release. |
514a291
to
c4cecf1
Compare
c4cecf1
to
d9d4bd4
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.
FYI to Truffle team, we no longer use bignumber.js or bn.js, just BigInts (@spacesailor24)
"lib": [ | ||
"es2017" | ||
], | ||
"lib": ["es2017", "dom"], |
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.
As mentioned web3/web3.js#6011 (comment), dom was added to handle the use of RequestInit, but there should be a better way to handle this on our end (@spacesailor24)
@@ -34,7 +34,7 @@ | |||
"@types/configstore": "^4.0.0", | |||
"@types/find-up": "^2.1.0", | |||
"@types/lodash": "^4.14.179", | |||
"@types/node": "~12.12.0", | |||
"@types/node": "~18.11.17", |
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.
Not sure this is acceptable (@spacesailor24)
@@ -7,7 +7,7 @@ import type * as Abi from "@truffle/abi-utils"; | |||
|
|||
import * as Import from "@truffle/codec/abi-data/import"; | |||
import * as AbiDataUtils from "@truffle/codec/abi-data/utils"; | |||
import Web3Utils from "web3-utils"; | |||
import * as Web3Utils from "web3-utils"; |
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.
@jdevcs We should probably fix this on our end and give them option to import everything under Web3Utils for backward compatibility (@spacesailor24)
@@ -2,7 +2,7 @@ const debug = require("debug")("test:util"); | |||
const fs = require("fs"); | |||
const ganache = require("ganache"); | |||
const Web3 = require("web3"); | |||
const Web3PromiEvent = require("web3-core-promievent"); | |||
const { Web3PromiEvent } = require("web3-core"); |
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 name of the const should probably be Web3Core instead of Web3PromiEvent, as the later use of new Web3PromiEvent.Web3PromiEvent(); doesn't make sense (@spacesailor24)
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.
It's used directly util.fakePromiEvent = new Web3PromiEvent();
, and we don't use anything else from web3-core
in the test.
@@ -127,13 +127,17 @@ module.exports = Contract => ({ | |||
// use that network and use latest block gasLimit | |||
if (this.network_id && this.networks[this.network_id] != null) { | |||
const { gasLimit } = await this.interfaceAdapter.getBlock("latest"); | |||
return { id: this.network_id, blockLimit: gasLimit }; | |||
return { id: this.network_id, blockLimit: String(gasLimit) }; |
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.
What is gasLimit being returned as? (@spacesailor24)
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.
number
id, | ||
result | ||
}); | ||
} | ||
} | ||
} | ||
}; | ||
} as Web3BaseProvider; |
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.
Not sure if we should type cast this if it's being used elsewhere as a non web3.js provider (@spacesailor24)
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.
Anything mentioned with as <something>
is probably fine. Usually used to point out one type from the union. If there is some real type issue we had to double typecast as unknown as <something>
which should be avoided.
@@ -64,7 +64,7 @@ const helpers = (db: Db, project: Project.Project) => ({ | |||
} | |||
}); | |||
|
|||
describe("Project.assignNames", () => { | |||
describe.skip("Project.assignNames", () => { |
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.
Unhandled TODO (@spacesailor24)
@@ -8,7 +8,7 @@ import * as Project from "@truffle/db/project"; | |||
import { resources, Run } from "@truffle/db/process"; | |||
import type { IdObject, Input, Resource } from "@truffle/db/resources"; | |||
|
|||
describe("Project.contractInstances", () => { | |||
describe.skip("Project.contractInstances", () => { |
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.
Unhandled TODO (@spacesailor24)
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.
It was failing earlier because of the following issue web3/web3.js#5714.
packages/db/src/test/source.spec.ts
Outdated
@@ -1,7 +1,7 @@ | |||
import { generateId, Migrations, WorkspaceClient } from "./utils"; | |||
import { AddSource, GetSource, GetAllSources } from "./source.graphql"; | |||
|
|||
describe("Source", () => { | |||
describe.skip("Source", () => { |
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.
Unhandled TODO (@spacesailor24)
@@ -10,19 +10,10 @@ | |||
"sourceMap": true, | |||
"outDir": "dist", | |||
"baseUrl": ".", | |||
"lib": ["es2019"], | |||
"lib": ["es2019", "dom"], |
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.
Related to previous comment (@spacesailor24)
const web3 = new Web3(); | ||
web3.setProvider(provider); | ||
|
||
const web3 = new Web3.Web3(provider); |
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.
This shouldn't be necessary (@spacesailor24)
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.
Yes not anymore.
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.
If we want to use it in the same way we have to change the import to const { Web3 } = require("web3");
,else I get the error TypeError: Web3 is not a constructor
@@ -28,7 +30,7 @@ const provider = Ganache.provider({ | |||
instamine: "strict" | |||
} | |||
}); | |||
const web3 = new Web3(); | |||
const web3 = new Web3.Web3(); |
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.
This shouldn't be necessary (@spacesailor24)
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.
same as above.
packages/contract/lib/handlers.js
Outdated
context.promiEvent.eventEmitter.emit("error", error); | ||
this.removeListener("error", handlers.error); | ||
context.promiEvent.emit("error", error); | ||
this.off("error", handlers.error); |
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.
Shouldn't removeListener work here? What was the issue? (@spacesailor24)
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.
Our new types for pomievent had off
and on
for event handling.
@@ -151,7 +151,9 @@ class ENS { | |||
|
|||
// Set top-level name | |||
let builtName = nameLabels[0]; | |||
await this.devRegistry.setSubnodeOwner("0x0", sha3(builtName), from, { | |||
const ZERO_NODE = | |||
"0x0000000000000000000000000000000000000000000000000000000000000000"; |
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.
Should remove if "0x0" suffices (@spacesailor24)
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.
Should be doubled backed with latest base branch.
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.
"0x0" fails the validation (Web3 validator found 1 error[s]: value "0x0" at "/0" must pass "bytes32" validation
)
packages/deployer/test/deployer.js
Outdated
assert(libReceipt.blockNumber === startBlock + 1); | ||
assert(exampleReceipt.blockNumber === libReceipt.blockNumber + 3); | ||
assert(libReceipt.blockNumber === startBlock + BigInt(1)); | ||
//todo web3js-migration this fails |
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.
Unhandled TODO (@spacesailor24)
//todo web3js-migration error withch schema | ||
// Error: schema with key or id "5f79e70fd2b8a3a84f947e1f2376b6311f756dc26ed9ebfe148d5b0ef3350ac3f933c17a46bbf7fa08c2edb8a39381688f05ed60ff4853d4a72ea3fed8a85b58" already exists |
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.
It now works after the updates to the validation function that is available in RC1.
However, the timeout needs to be extended. And this could be done for example by calling inside the test function: this.timeout(3000);
. Or by calling on the test function itself like in:
it("...", async function () {
...
}).timeout(3000);
Hello! What is the status of this PR? Is there any chance you can rebase this? I would be happy to do the rebase if you'd like as it appears to be dependency-related things which hopefully will be easy to handle. Let me know! |
Hello @eggplantzzz |
Sorry for the delay @nazarhussain. Things have been shifting around here quite a bit and I've been preoccupied. I will do the rebase soon! Update: it looks like I'm having trouble rebasing this PR as I think you are using a different npm registry for some unpublished packages, is that right? Also there were some dependencies that I think require Node >= 16 while current policy for Truffle is to support versions of Node as early as the latest version of 12. Is there any way we can get around this issue? It is failing after updating package.json's, regenerating the yarn.lock, and rebuilding. The library that the error is coming from is @noble/hashes. Update2: we might actually be able to drop support for Node 14 in the near future, so that actually might not be a problem after we get to that point. We'll have to see how early we can do that. |
Thanks @eggplantzzz,
However, there are still some skipped tests and TODOs. Could you also please manage to run the CI for this MR? I suggest creating a feature branch at this repo and we merge this MR to that branch. If this would be helpful for both, running the CI and easing your collaboration on this. Many thanks, |
That will be supper cool and will help to maintain Truffle. @Muhammad-Altabba @eggplantzzz My suggestion would be to look keenly into skipped tests and if these are not critical, fix those in separate PRs, so we can get this big PR merged earlier. |
Ok, let me switch to Node 16 and try the rebase again. Also, do note @nazarhussain @Muhammad-Altabba that the command |
Thanks @eggplantzzz
Thanks, |
@eggplantzzz Thanks for your efforts, the web3js team appreciates you :) I tried to understand the error but im still not sure completely what is wrong. But this issue is coming from web3-eth@4.1.0, which are the newest web3js release packages. While I debug the issue with the build, |
So I do have all packages bumped to |
and update Web3 imports
df37368
to
a674046
Compare
(fix related to web3/web3.js#6312)
No description provided.