-
Notifications
You must be signed in to change notification settings - Fork 58
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
Signature verification #549
Conversation
Looks good lets ship it. |
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## main #549 +/- ##
==========================================
- Coverage 29.75% 29.64% -0.11%
==========================================
Files 48 48
Lines 7045 7046 +1
==========================================
- Hits 2096 2089 -7
- Misses 4768 4773 +5
- Partials 181 184 +3
|
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.
Some comments and questions.
@@ -33,13 +33,9 @@ orchestrator = { path = "./orchestrator" } | |||
cosmos_gravity = { path = "./cosmos_gravity" } | |||
ethereum_gravity = { path = "./ethereum_gravity" } | |||
gravity_utils = { path = "./gravity_utils" } | |||
gravity_proto_build = { path = "./gravity_proto_build" } |
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.
The cargo setup here is structured as a workspace. These aren't unused dependencies, their local Cargo.toml
files inform the shared overall Cargo.lock
of the workspace. They won't build if they aren't included 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.
cargo ignores them anyway and throws warnings that they are invalid dependencies because nothing actually depends on them. they have to be built separately
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.
Ah, I confused this section with the members
section.
@@ -15,20 +15,14 @@ cosmos_gravity = {path = "../cosmos_gravity"} | |||
gravity_abi = { path = "../gravity_abi" } | |||
gravity_utils = {path = "../gravity_utils"} | |||
gravity_proto = {path = "../gravity_proto/"} | |||
orchestrator = {path = "../orchestrator/"} |
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.
Hadn't we planned to delete the test_runner
crate entirely in favor of the integration tests? I guess we only did that for the old testnet directory for the module and not the orchestrator?
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 thought so too, happy to delete if you think we should
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 can do it later.
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.
LGTM
No description provided.