-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gre: refactor to allow using L1 and L2 simultaneously #682
Conversation
Signed-off-by: Tomás Migone <[email protected]>
Signed-off-by: Tomás Migone <[email protected]>
Signed-off-by: Tomás Migone <[email protected]>
Looking good |
Signed-off-by: Tomás Migone <[email protected]>
686347e
to
632e184
Compare
Signed-off-by: Tomás Migone <[email protected]>
Signed-off-by: Tomás Migone <[email protected]>
Signed-off-by: Tomás Migone <[email protected]>
Signed-off-by: Tomás Migone <[email protected]>
Signed-off-by: Tomás Migone <[email protected]>
Codecov Report
@@ Coverage Diff @@
## dev #682 +/- ##
=======================================
Coverage 90.35% 90.35%
=======================================
Files 35 35
Lines 1762 1762
Branches 296 296
=======================================
Hits 1592 1592
Misses 170 170
Flags with carried forward coverage won't be shown. Click here to find out more. Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Congrats @tmigone this is looking great
@@ -0,0 +1,5 @@ | |||
{ | |||
"require": "ts-node/register/files", |
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 this? :)
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.
I knew you would ask ;)
The plugin tests run with mocha (and not with hh test), this is needed for mocha to support typescript files.
On a more general note, I based the GRE folder on hardhat's plugin boilerplate https://github.com/NomicFoundation/hardhat-ts-plugin-boilerplate/, once GRE is mature I think it makes sense to fork it into it's own repository so each repo is nice and tidy and not polluted with too much stuff.
l1ChainId, | ||
l2ChainId, | ||
isHHL1, | ||
isHHL2, |
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.
Is the isHHL2 ever used? as the first isHHL1 is binary. Or this is just included for symmetry?
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.
No, isHHL2
is not used (though you could use it down the line). Just included as you say for symmetry. Perhaps we can remove it and rename the isHHL1
variable to something else but I struggled to find a good name.
@@ -91,6 +91,7 @@ | |||
"test:e2e": "scripts/e2e", | |||
"test:gas": "RUN_EVM=true REPORT_GAS=true scripts/test", | |||
"test:coverage": "scripts/coverage", | |||
"test:gre": "cd gre && mocha --exit --recursive 'test/**/*.test.ts' && cd ..", |
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.
💯
@tmigone what are the empty config files under |
Exactly. The tests are mainly to verify the plugin does path resolution correctly since there are a lot of cases to evaluate (and files need to exist to avoid erroring out). The contents of the files don't really matter as here we don't go that far so I figured it was better to leave them empty. |
Fixes GRT-8 |
This PR adds support to GRE to allow using L1 and L2 chains simultaneously. This greatly simplifies interacting with the protocol.
Note that this is on hardhat's roadmap (see NomicFoundation/hardhat#2797) but it's going to take a while before it's ready.
Relevant docs with more information:
gre
folderFixes GRT-8