Skip to content
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

Merged
merged 9 commits into from
Aug 23, 2022

Conversation

tmigone
Copy link
Contributor

@tmigone tmigone commented Aug 12, 2022

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:

Fixes GRT-8

@tmigone tmigone marked this pull request as draft August 12, 2022 13:49
@abarmat
Copy link
Contributor

abarmat commented Aug 15, 2022

Looking good

Signed-off-by: Tomás Migone <[email protected]>
@tmigone tmigone force-pushed the tmigone/gre-multi-layer branch from 686347e to 632e184 Compare August 16, 2022 13:13
Signed-off-by: Tomás Migone <[email protected]>
Signed-off-by: Tomás Migone <[email protected]>
@tmigone tmigone changed the title [WIP] gre: refactor to allow using L1 and L2 simultaneously gre: refactor to allow using L1 and L2 simultaneously Aug 16, 2022
@tmigone tmigone marked this pull request as ready for review August 16, 2022 17:36
@tmigone tmigone requested a review from abarmat August 17, 2022 13:41
@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #682 (37cddd3) into dev (5232fd3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev     #682   +/-   ##
=======================================
  Coverage   90.35%   90.35%           
=======================================
  Files          35       35           
  Lines        1762     1762           
  Branches      296      296           
=======================================
  Hits         1592     1592           
  Misses        170      170           
Flag Coverage Δ
unittests 90.35% <ø> (ø)

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.

Copy link
Contributor

@abarmat abarmat left a 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",
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this? :)

Copy link
Contributor Author

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,
Copy link
Contributor

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?

Copy link
Contributor Author

@tmigone tmigone Aug 23, 2022

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 ..",
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@abarmat
Copy link
Contributor

abarmat commented Aug 23, 2022

@tmigone what are the empty config files under gre/test/files, are those for existence tests?

@tmigone
Copy link
Contributor Author

tmigone commented Aug 23, 2022

@tmigone what are the empty config files under gre/test/files, are those for existence tests?

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.

@tmigone tmigone merged commit 707b856 into dev Aug 23, 2022
@tmigone tmigone deleted the tmigone/gre-multi-layer branch August 23, 2022 12:13
@abarmat
Copy link
Contributor

abarmat commented Aug 23, 2022

Fixes GRT-8

@linear
Copy link

linear bot commented Aug 23, 2022

GRT-8 Multi-chain GRE

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

Successfully merging this pull request may close these issues.

2 participants