Switch ENS external test to ens-contracts repo #12198

cameel · 2021-10-27T11:06:52Z

Fixes #8557.
~~Depends on #12192 (draft until that PR is merged).~~ Merged.

ens-contracts seems to be the new consolidated repo for all ENS contracts. The project uses Hardhat now and is compatible with Solidity >= 0.8.4.

The test also now uses the upstream directly rather than our fork in solidity-external-tests.

cameel · 2021-10-27T15:37:06Z

This requires a bit more work. I got Hardhat to compile the project with the custom compiler but @ensdomains in node_modules/ still gets compiled using 0.8.4 (even though I'm replacing all the copies of solc-js installed by npm).

leonardoalt · 2021-11-03T10:48:24Z

Is this needed after #12187 ?

cameel · 2021-11-03T11:35:54Z

Yes. Both update the repo URL but #12187 is just a trivial switch from our fork to the upstream (which just works, without any other changes needed so it can be merged right away) while this PR switches to a different repo and requires more work. Also depends on other PRs. ENS has moved all of their contracts into a single repo one and switched from Truffle to Hardhat.

I still need to fix one problem here - some dependency is being compiled with older solc, triggering our compiler version check.

axic · 2021-11-08T16:00:52Z

test/externalTests/ens.sh


 function ens_test
 {
-    local repo="https://github.com/ensdomains/ens.git"
+    local repo="https://github.com/ensdomains/ens-contracts.git"
    local branch=master


Not sure about this, I think it is heavily in flux, so a tagged version would be better.

Oh. You mean an upstream tag or a full-blown fork in solidity-external-tests?

Upstream tag, or even just commit hash.

Looks like the tag I used (v0.0.5) is too ancient and lots of tests fail. Switching to a specific commit.

Looks like git clone does not work with specific commits. The only thing I could do is to clone the whole repo (without --depth 1) and then check out the commit. I'd need to add a separate function to handle that case. I can do it but it's a bit of a hassle. How unstable is their master really?

So what do we do about it? Can it stay at master after all?

There's a new upstream tag dated yesterday: https://github.com/ensdomains/ens-contracts/tags

Oh, nice. I even checked the repo today but I missed it because I only looked at releases.

In that case I'll switch to that and then let's get this merged :)

cameel · 2021-12-06T17:39:29Z

This is now reviewable.

test/externalTests/common.sh

test/externalTests/ens.sh

bshastry · 2021-12-21T10:46:49Z

test/externalTests/ens.sh

-    local compile_only_presets=()
+    local compile_only_presets=(
+        legacy-no-optimize        # Compiles but tests fail to deploy GovernorCompatibilityBravo (code too large).
+    )


Why is it okay to tolerate test failures due to large code size (presumably due to no optimization)?

In this case it's really because I don't want to spend time on tweaking their test/ganache config for this to pass and the this particular config preset is not all that important for us anyway - we skip it in many tests due to stack-related errors. I'd prioritize it more if it was the only preset that could work or if it was the legacy-optimize-evm+yul one.

test/externalTests/ens.sh

cameel · 2021-12-21T12:48:36Z

Ready for another round or review.

bshastry · 2021-12-21T14:39:51Z

Ready for another round or review.

lint of solcjs is failing though.

cameel · 2021-12-21T17:40:59Z

Failing job was unrelated, fixed in #12446. I just rebased to make tests pass.

bshastry

LGTM. What is missing consensus is whether to test the master branch (unstable), a specific commit etc. So I'd wait for what @axic has to say before merging this PR.

cameel · 2021-12-22T10:53:28Z

Fair enough. I'd be inclined to just track their master and see how often it breaks. We can always switch to a fork if it happens too often.

Tracking a specific commit will require some slight adjustments because it cannot be done after git clone --depth 1. Would be best if all of these projects had sensible tags but the ones where they would be most helpful are ones that are missing them :)

Also, ENS is not the only project having this problem. Trident is also developing quite rapidly.

…o-ens-contracts Switch ENS external test to ens-contracts repo

cameel requested review from bshastry and chriseth October 27, 2021 11:06

cameel self-assigned this Oct 27, 2021

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch from 624cd8c to 08162f3 Compare October 27, 2021 14:36

cameel force-pushed the hardhat-in-oz-ext-test branch from 5b0a006 to cb07ab0 Compare October 27, 2021 15:01

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch from 08162f3 to 38d8ea5 Compare October 27, 2021 15:32

cameel force-pushed the hardhat-in-oz-ext-test branch from cb07ab0 to 7058281 Compare October 27, 2021 19:18

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch from 38d8ea5 to ef20048 Compare October 29, 2021 00:07

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch 2 times, most recently from 6065758 to 94db5ac Compare November 5, 2021 16:02

cameel force-pushed the hardhat-in-oz-ext-test branch 3 times, most recently from f4d9f78 to df17733 Compare November 5, 2021 17:05

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch 2 times, most recently from 625af4c to 47c56a6 Compare November 5, 2021 18:14

axic reviewed Nov 8, 2021

View reviewed changes

cameel force-pushed the hardhat-in-oz-ext-test branch from df17733 to c33cd75 Compare November 9, 2021 15:57

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch from 47c56a6 to 36b7d0a Compare November 9, 2021 16:17

cameel force-pushed the hardhat-in-oz-ext-test branch from c33cd75 to 5ef7962 Compare November 24, 2021 12:46

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch from 280f9d0 to 25230e9 Compare November 24, 2021 12:46

cameel force-pushed the hardhat-in-oz-ext-test branch from 5ef7962 to 9f59d17 Compare November 25, 2021 20:54

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch from 25230e9 to 1ee5162 Compare November 25, 2021 20:54

Base automatically changed from hardhat-in-oz-ext-test to develop November 30, 2021 15:17

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch 2 times, most recently from 8a92d20 to 2cc8992 Compare December 6, 2021 17:38

cameel marked this pull request as ready for review December 6, 2021 17:39

cameel added ext tests testing 🔨 and removed ext tests labels Dec 10, 2021

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch from 2cc8992 to d871831 Compare December 13, 2021 12:31

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch from d871831 to 54d8a6b Compare December 20, 2021 19:17

bshastry reviewed Dec 21, 2021

View reviewed changes

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch from 54d8a6b to 60939bd Compare December 21, 2021 12:47

cameel requested review from bshastry and axic December 21, 2021 12:48

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch from 60939bd to 9148e92 Compare December 21, 2021 17:40

bshastry previously approved these changes Dec 22, 2021

View reviewed changes

cameel dismissed bshastry’s stale review via 836fe57 December 22, 2021 13:31

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch from 9148e92 to 836fe57 Compare December 22, 2021 13:31

Switch ens external test to ens-contracts repo

7e91dba

cameel force-pushed the switch-ens-ext-test-to-ens-contracts branch from 836fe57 to 7e91dba Compare December 22, 2021 13:46

bshastry approved these changes Dec 22, 2021

View reviewed changes

cameel merged commit b28cd00 into develop Dec 22, 2021

cameel deleted the switch-ens-ext-test-to-ens-contracts branch December 22, 2021 15:38

nishant-sachdeva pushed a commit to nishant-sachdeva/solidity that referenced this pull request Dec 24, 2021

Merge pull request ethereum#12198 from ethereum/switch-ens-ext-test-t…

4e36c3c

…o-ens-contracts Switch ENS external test to ens-contracts repo

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch ENS external test to ens-contracts repo #12198

Switch ENS external test to ens-contracts repo #12198

cameel commented Oct 27, 2021 •

edited

Loading

cameel commented Oct 27, 2021

leonardoalt commented Nov 3, 2021

cameel commented Nov 3, 2021 •

edited

Loading

axic Nov 8, 2021

cameel Nov 8, 2021

axic Nov 8, 2021

cameel Nov 9, 2021

cameel Nov 9, 2021

cameel Nov 9, 2021

cameel Dec 6, 2021

bshastry Dec 22, 2021

cameel Dec 22, 2021

cameel commented Dec 6, 2021

bshastry Dec 21, 2021

cameel Dec 21, 2021

cameel commented Dec 21, 2021

bshastry commented Dec 21, 2021

cameel commented Dec 21, 2021

bshastry left a comment

cameel commented Dec 22, 2021

Switch ENS external test to ens-contracts repo #12198

Switch ENS external test to ens-contracts repo #12198

Conversation

cameel commented Oct 27, 2021 • edited Loading

cameel commented Oct 27, 2021

leonardoalt commented Nov 3, 2021

cameel commented Nov 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel commented Dec 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cameel commented Dec 21, 2021

bshastry commented Dec 21, 2021

cameel commented Dec 21, 2021

bshastry left a comment

Choose a reason for hiding this comment

cameel commented Dec 22, 2021

cameel commented Oct 27, 2021 •

edited

Loading

cameel commented Nov 3, 2021 •

edited

Loading