-
Notifications
You must be signed in to change notification settings - Fork 18
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
chore: rewrite trin-bridge simulator increasing test accuracy + test automation #115
Conversation
fa7f8fc
to
a618981
Compare
780384a
to
7335712
Compare
7335712
to
20f727c
Compare
20f727c
to
047daad
Compare
This PR is now unblocked and ready to review |
047daad
to
3e03576
Compare
I'm glad you mentioned this. Can you also open a PR against the ansible configs to reduce breakage here? Might need to talk to Paul to make sure those new configs are deployed (I've never handled portal-hive deploys) cc flamingo @ogenev I think this is an example of a line that has to change, but you better confirm with Paul: |
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 is mostly a rubber-stamp. I scanned it, and it generally looks good to me! 👍🏻
I'm assuming you tried it out locally. Just be sure to coordinate with Paul so that portal-hive doesn't break, because of the new CLI arg requirement.
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.
@@ -0,0 +1,14 @@ | |||
// Execution Layer hard forks https://ethereum.org/en/history/ |
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 already have these constants over inside portal-interop/src/constants.rs
, is there any way we can import those? If not, I'd recommend moving these to a single location where they can be imported wherever they're needed in the codebase rather than copying the constants multiple times
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.
Or this one?
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.
Going to handle it in a follow up PR. Currently there is no good one place to put this.
So I am thinking of hosting a crate on ethereum/portal-spec-tests with test utils crate. A issue with this is then we have to update that repo to use new utils in this repo. So it would only be used for non-simulator-specific constants, and be updated less frequently to eventually deduplicate constants.
We have hivesim-rust, but I wouldn't put these constants there as these are not related and contamination is bad.
And there isn't really a good place to declare a crate in this repo for common utils expessially with our goals of being re-included into ethereum/hive, breaking the projects structure would be a no no.
This request didn't really feel like a blocker to merge because of the reasons above more of a nit-pick which doesn't have the highest priority. Well yes it is nice to have no duplication of common constants between simulators, this PR isn't the place to change that. And these simulators have been rewritten almost 3 times now in how we structure tests, so where we put constants is the last of my worries.
I was planning to do a follow up PR relatively soon after I incorporated using ethereum/portal-spec-tests repo, to which would have answered this question. Didn't expect such a quick turnaround xD
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.
Sounds good, in the future it's worth mentioning how you're going to resolve a comment before hitting the resolve
button, otherwise to the other person it looks like you just ignored it. Also, it gives a chance for the other person to have input into the planned resolution.
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.
Sounds good 👍
This PR is now ready to review
#110 (comment)
#110 (comment)
#110 (comment)
Why this PR was made can be read in the comments linked to above.
This PR will in short more accurately test trin-bridge by testing the docker build of the latest bridge release, instead of using the bridge as a library.
In theory we can now create a mock pandaops and do backfill tests and latest tests now if we wanted to, but this is the start to make this all possible.
Important note
To run this simulator you will have to include
trin-bridge
as a client example?./hive --sim trin-bridge --client trin,trin-bridge
If you forget to add this the simulator will panic asking for you to add this. Because of how hive was designed this is required. In the future we can probably remove this requirement, but it is not priority and it should be done after we transition from
ethereum/portal-hive
->ethereum/hive
. Since this will be a change modify the hive code itself so I want it reviewed by them whenever that time comes.