-
Notifications
You must be signed in to change notification settings - Fork 107
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
Improve e2e scripts #855
Improve e2e scripts #855
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #855 +/- ##
===========================================
- Coverage 74.65% 64.44% -10.21%
===========================================
Files 28 10 -18
Lines 1294 225 -1069
Branches 0 52 +52
===========================================
- Hits 966 145 -821
+ Misses 328 60 -268
- Partials 0 20 +20 ☔ View full report in Codecov by Sentry. |
@@ -40,7 +40,7 @@ | |||
|
|||
# ethereum | |||
foundry-bin | |||
go-ethereum | |||
# go-ethereum |
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.
@doubledup can you try bump the version in Nix again please.
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.
Actually @yrong, is it necessary to upgrade to go-ethereum 1.12? Can we wait for a bit longer?
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.
Seems nix packages for 1.12 is not ready yet, For upstreams components we heavily depend on like Geth I would prefer to sync more closely to recent release without waiting nix counterpart to be ready.
Btw, Geth client used here is only for dev setup and I've tested E2E still work as expected. For production usage we can just connect to infura endpoint as we did before.
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.
ok, sounds fine 👍
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.
We would still need to run geth for the beacon node to connect to an Eth1 chain though, right?
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.
Exactly, but I would prefer these locally running clients are only readable for fast api response(reading from public endpoint could be slow), for transactions we can still submit to the infura endpoint as before.
core/packages/test/deployment/goerli-cd9e04958cb72e3fb1026c80b0e6ed032c9e83a6-1686329993.json
Outdated
Show resolved
Hide resolved
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.
+1
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.
Looks great! I'll test this out as soon as my local setup works again.
@@ -7,14 +7,14 @@ source scripts/xcm-helper.sh | |||
config_beacon_checkpoint() | |||
{ | |||
pushd $root_dir | |||
check_point_call=$($relay_bin generate-beacon-checkpoint --spec $active_spec --url $beacon_endpoint_http) | |||
local check_point_call=$($relay_bin generate-beacon-checkpoint --spec $active_spec --url $beacon_endpoint_http) |
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.
Why are you adding this? Just a best practice? 😄 Or did you encounter problems because the variable isn't local?
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, just best practice Vincent mentioned here #855 (comment)
@@ -40,7 +40,7 @@ | |||
|
|||
# ethereum | |||
foundry-bin | |||
go-ethereum | |||
# go-ethereum |
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.
We would still need to run geth for the beacon node to connect to an Eth1 chain though, right?
Refactoring including