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(devnet): add ISMP #270

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

feat(devnet): add ISMP #270

wants to merge 8 commits into from

Conversation

peterwht
Copy link
Collaborator

@peterwht peterwht commented Sep 10, 2024

This PR adds ISMP to the devnet runtime. The purpose is to get ISMP added to devnet to start deeper work to interface ISMP with contracts, and create more comprehensive testing.

It includes:

Devnet runtime:

  • ISMP pallet
  • removed: Demo pallet

Node

  • ISMP Inherent
  • ISMP RPC

Test

  • removed: Script to test GET request + response

Note:
In order to use ISMP, it must be built with --features ismp enabled. When this feature is enabled, both testnet and mainnet runtimes will be disabled and error with "ISMP is not enabled". If the feature is not enabled, devnet, testnet, and mainnet will be built. However, ISMP will not work for devnet.

Because the node requires that the runtimes implement the runtime API, we must feature-gate with the ISMP feature. Unfortunately, because trait aliases are experimental (nightly) feature-gating is rather verbose allowing us to remain on stable.

Updated CI PR here: #276

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2024

Codecov Report

Attention: Patch coverage is 0% with 129 lines in your changes missing coverage. Please review.

Project coverage is 51.47%. Comparing base (b653091) to head (dc56b5f).

Files with missing lines Patch % Lines
runtime/devnet/src/lib.rs 0.00% 64 Missing ⚠️
node/src/rpc.rs 0.00% 24 Missing ⚠️
node/src/service.rs 0.00% 22 Missing ⚠️
runtime/devnet/src/config/ismp.rs 0.00% 9 Missing ⚠️
node/src/chain_spec.rs 0.00% 8 Missing ⚠️
node/src/command.rs 0.00% 2 Missing ⚠️
@@            Coverage Diff             @@
##             main     #270      +/-   ##
==========================================
- Coverage   52.22%   51.47%   -0.76%     
==========================================
  Files          47       48       +1     
  Lines        4823     4894      +71     
  Branches     4823     4894      +71     
==========================================
  Hits         2519     2519              
- Misses       2255     2326      +71     
  Partials       49       49              
Files with missing lines Coverage Δ
node/src/command.rs 0.00% <0.00%> (ø)
node/src/chain_spec.rs 9.47% <0.00%> (-0.26%) ⬇️
runtime/devnet/src/config/ismp.rs 0.00% <0.00%> (ø)
node/src/service.rs 0.00% <0.00%> (ø)
node/src/rpc.rs 0.00% <0.00%> (ø)
runtime/devnet/src/lib.rs 5.53% <0.00%> (-0.92%) ⬇️

@peterwht
Copy link
Collaborator Author

[sc-151]

Copy link
Collaborator

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

LGTM, but my preference would be to not merge in the demo stuff, but rather separate it out into a branch/draft PR which can be used for any testing required and as an easy reference for our own implementation.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
node/src/chain_spec.rs Show resolved Hide resolved
node/src/service.rs Show resolved Hide resolved
node/src/service.rs Outdated Show resolved Hide resolved
runtime/devnet/src/config/ismp.rs Outdated Show resolved Hide resolved
runtime/devnet/src/config/ismp.rs Show resolved Hide resolved
runtime/devnet/src/config/ismp.rs Outdated Show resolved Hide resolved
runtime/devnet/src/lib.rs Outdated Show resolved Hide resolved
scripts/ismp-get/README.md Outdated Show resolved Hide resolved
@peterwht
Copy link
Collaborator Author

To document here, the demo pallet was removed from this PR. Testing ISMP can utilize the following branch (which will never be merged into main). #293

Copy link
Collaborator

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

Just one last tweak please - by adding the mentioned pallet into the benchmarks list now we should hopefully not miss it when progressing to testnet, provided we do a simple diff of the runtimes.

runtime/devnet/src/config/ismp.rs Show resolved Hide resolved
Copy link
Collaborator

@Daanvdplas Daanvdplas left a comment

Choose a reason for hiding this comment

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

Very exciting! Left one comment but no reason to wait with merging

runtime/devnet/src/lib.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants