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

feat: runtime metadata hash verification #5467

Merged
merged 13 commits into from
Dec 11, 2024

Conversation

ramizhasan111
Copy link
Contributor

@ramizhasan111 ramizhasan111 commented Dec 4, 2024

Pull Request

Closes: PRO-1733

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have written sufficient tests.
  • I have written and tested required migrations.
  • I have updated documentation where appropriate.

Summary

Please include a succinct description of the purpose and content of the PR. What problem does it solve, and how? Link issues, discussions, other PRs, and anything else that will help the reviewer.

Non-Breaking changes

If this PR includes non-breaking changes, select the non-breaking label. On merge, CI will automatically cherry-pick the commit to a PR against the release branch.

@ramizhasan111 ramizhasan111 marked this pull request as ready for review December 6, 2024 14:52
Copy link
Collaborator

@dandanlen dandanlen left a comment

Choose a reason for hiding this comment

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

It would be nice to have a bouncer test that uses the metadata hash - we can add this separately.

.cargo/config.toml Outdated Show resolved Hide resolved
state-chain/node/Cargo.toml Outdated Show resolved Hide resolved
.cargo/config.toml Outdated Show resolved Hide resolved
.github/workflows/ci-development.yml Outdated Show resolved Hide resolved
state-chain/runtime/build.rs Show resolved Hide resolved
state-chain/runtime/src/lib.rs Outdated Show resolved Hide resolved
@ramizhasan111
Copy link
Contributor Author

It would be nice to have a bouncer test that uses the metadata hash - we can add this separately.

Yes, that would be nice.
I did test it locally on a localnet enabling the metadata hash verification for engine state chain client and providing the metadata hash in the engine. Works fine 👌

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 72%. Comparing base (d12200a) to head (8e3fcee).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
.../client/extrinsic_api/signed/submission_watcher.rs 0% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #5467    +/-   ##
======================================
- Coverage     72%     72%    -0%     
======================================
  Files        494     495     +1     
  Lines      87751   88004   +253     
  Branches   87751   88004   +253     
======================================
+ Hits       62826   62971   +145     
- Misses     22310   22453   +143     
+ Partials    2615    2580    -35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@dandanlen dandanlen left a comment

Choose a reason for hiding this comment

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

LGTM - I couldn't resist the urge to sort the dependencies though 🙈

Also, just confirm - right now we never build the runtime with the extension enabled. I expect that we want to add this to the release or production profile at some point. I think we can discuss this later depending on product / testing concerns.

@dandanlen dandanlen enabled auto-merge December 11, 2024 12:18
@dandanlen dandanlen added this pull request to the merge queue Dec 11, 2024
Merged via the queue into main with commit 5a4bc00 Dec 11, 2024
48 checks passed
@dandanlen dandanlen deleted the feat/runtime-metadata-hash-verification branch December 11, 2024 13:43
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