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

chore: comment out ismp #414

Closed
wants to merge 3 commits into from
Closed

Conversation

Daanvdplas
Copy link
Collaborator

@Daanvdplas Daanvdplas commented Dec 18, 2024

In order to uplift pop to stable 2412 the ismp related code will be commented out. This is because the ismp dependencies are currently at 2407 and there is no eta when they will do those upgrades. In the meantime we can proceed with the upgrades and relevant work depending on this.

As for the messaging feature that is in the pipeline, I created a branch from main: feat/messaging. This branch is on v1.14 with ismp still in the devnet runtime.

Before reviewing, see latest commit for something that was required to make zepter happy.

@Daanvdplas Daanvdplas requested review from peterwht, al3mart and chungquantin and removed request for al3mart, peterwht and chungquantin December 18, 2024 20:15
@Daanvdplas Daanvdplas added bug Something isn't working blocked labels Dec 18, 2024
@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 68.37%. Comparing base (3476994) to head (855146b).

Files with missing lines Patch % Lines
runtime/devnet/src/lib.rs 0.00% 61 Missing ⚠️
node/src/chain_spec.rs 0.00% 6 Missing ⚠️
node/src/service.rs 0.00% 1 Missing ⚠️
@@            Coverage Diff             @@
##             main     #414      +/-   ##
==========================================
- Coverage   68.41%   68.37%   -0.05%     
==========================================
  Files          70       69       -1     
  Lines       11838    11845       +7     
  Branches    11838    11845       +7     
==========================================
  Hits         8099     8099              
- Misses       3482     3489       +7     
  Partials      257      257              
Files with missing lines Coverage Δ
node/src/command.rs 0.00% <ø> (ø)
node/src/rpc.rs 0.00% <ø> (ø)
node/src/service.rs 0.00% <0.00%> (ø)
node/src/chain_spec.rs 9.53% <0.00%> (+0.06%) ⬆️
runtime/devnet/src/lib.rs 5.53% <0.00%> (ø)

... and 1 file with indirect coverage changes

@peterwht
Copy link
Collaborator

before approving this, I think it's worthwhile to see if we can revert back to our ISMP fork and update that repo as well. In the past it's been smooth with only PSVM being required.

See this commit: r0gue-io/ismp@46862a0

IMPORTANT NOTE:

The ISMP repo will NOT build standalone. You will verify its compilation through building pop-node with the ISMP modules pointing to your new ISMP branch. This is because we are only using the ISMP pallet.

Copy link
Collaborator

@chungquantin chungquantin left a comment

Choose a reason for hiding this comment

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

If everything related to ISMP removed, why don't we fork main branch to ismp, create a PR and then remove everything related to ismp in main. This avoid leaving all the commented-out code in everywhere in the file.

Edit: Saw you removed review requests, I'll leave my comments here anyway.

- name: Check Build with ISMP
run: |
cargo check --release --locked --features=ismp,runtime-benchmarks,try-runtime
# check-ismp:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this has been merged into main, it's worth leaving clear TODOs for the commented-out code.

@Daanvdplas Daanvdplas removed bug Something isn't working blocked labels Dec 19, 2024
@Daanvdplas Daanvdplas closed this Dec 23, 2024
@Daanvdplas Daanvdplas deleted the daan/refactor-comment_out_ismp branch December 23, 2024 12:07
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.

4 participants