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

fix: hrmp #278

Merged
merged 25 commits into from
Dec 19, 2024
Merged

fix: hrmp #278

merged 25 commits into from
Dec 19, 2024

Conversation

evilrobot-01
Copy link
Contributor

@evilrobot-01 evilrobot-01 commented Aug 7, 2024

Ensures hrmp channels work seamlessly via pop-cli.

Requires v1.3.3+ of the Paseo chain spec generator.

Test using:
cargo run -- up parachain -f ./tests/networks/pop.toml --verbose

Copy link

codecov bot commented Aug 7, 2024

Codecov Report

Attention: Patch coverage is 73.74302% with 47 lines in your changes missing coverage. Please review.

Project coverage is 75.11%. Comparing base (22a74b3) to head (863ab92).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
crates/pop-cli/src/commands/up/parachain.rs 0.00% 28 Missing ⚠️
crates/pop-parachains/src/relay.rs 83.51% 15 Missing ⚠️
crates/pop-parachains/src/up/mod.rs 92.85% 0 Missing and 4 partials ⚠️
@@            Coverage Diff             @@
##             main     #278      +/-   ##
==========================================
+ Coverage   75.10%   75.11%   +0.01%     
==========================================
  Files          61       62       +1     
  Lines       13701    13869     +168     
  Branches    13701    13869     +168     
==========================================
+ Hits        10290    10418     +128     
- Misses       2056     2094      +38     
- Partials     1355     1357       +2     
Files with missing lines Coverage Δ
crates/pop-cli/src/commands/call/chain.rs 74.64% <100.00%> (+0.11%) ⬆️
crates/pop-parachains/src/call/mod.rs 58.33% <100.00%> (+0.59%) ⬆️
crates/pop-parachains/src/utils/helpers.rs 79.77% <ø> (ø)
crates/pop-parachains/src/up/mod.rs 87.59% <92.85%> (+0.16%) ⬆️
crates/pop-parachains/src/relay.rs 83.51% <83.51%> (ø)
crates/pop-cli/src/commands/up/parachain.rs 7.00% <0.00%> (-0.61%) ⬇️

... and 1 file with indirect coverage changes

@evilrobot-01
Copy link
Contributor Author

Rebased.

@evilrobot-01 evilrobot-01 marked this pull request as ready for review November 27, 2024 14:41
@evilrobot-01 evilrobot-01 added the ready-for-high-level-review The PR needs a high level review label Nov 27, 2024
@Daanvdplas
Copy link
Collaborator

I noticed that when I build the chain-spec-generator and used it to spin up a local network the parachains were not producing blocks. Used the release binary and it worked. Not 100% sure I did something wrong or it is the chain spec generator. Just put it here so that we are aware.

@AlexD10S AlexD10S self-requested a review December 3, 2024 13:11
Copy link
Collaborator

@AlexD10S AlexD10S left a comment

Choose a reason for hiding this comment

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

Looks good to me from a high-level perspective. Makes sense to replace https://github.com/r0gue-io/paseo-runtimes with https://github.com/r0gue-io/paseo-network-runtimes and begin maintaining it.

crates/pop-parachains/src/relay.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/up/parachain.rs Outdated Show resolved Hide resolved
@evilrobot-01
Copy link
Contributor Author

Makes sense to replace https://github.com/r0gue-io/paseo-runtimes with https://github.com/r0gue-io/paseo-network-runtimes and begin maintaining it.

Need to coordinate with Ale in terms of whether this could be upstreamed to Paseo repo, even if with some feature flag.

@al3mart
Copy link
Contributor

al3mart commented Dec 9, 2024

Makes sense to replace https://github.com/r0gue-io/paseo-runtimes with https://github.com/r0gue-io/paseo-network-runtimes and begin maintaining it.

Need to coordinate with Ale in terms of whether this could be upstreamed to Paseo repo, even if with some feature flag.

I was reviewing the changes we made. I can confirm that now everything on Paseo' side is using sr25519. As for the hrmp at genesis that can and should be pushed upstream! There's no point on us keeping a fork if there's not that many changes.

I can take care of doing so 🤝

@evilrobot-01
Copy link
Contributor Author

As for the hrmp at genesis that can and should be pushed upstream! There's no point on us keeping a fork if there's not that many changes.

I can take care of doing so 🤝

Should just be whatever is applicable at paseo-network/runtimes@main...r0gue-io:paseo-network-runtimes:pop. It would be great to get this into the next pop release, but that would require a Paseo release with the updated CSG.

@al3mart
Copy link
Contributor

al3mart commented Dec 10, 2024

@evilrobot-01 v1.3.3 and v1.3.4 should present no blockers now.

@evilrobot-01 evilrobot-01 removed the ready-for-high-level-review The PR needs a high level review label Dec 16, 2024
@evilrobot-01
Copy link
Contributor Author

Rebased

crates/pop-cli/src/commands/up/parachain.rs Outdated Show resolved Hide resolved
tests/networks/pop.toml Show resolved Hide resolved
crates/pop-cli/src/commands/up/parachain.rs Show resolved Hide resolved
crates/pop-cli/src/commands/up/parachain.rs Show resolved Hide resolved
crates/pop-cli/src/commands/up/parachain.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/up/parachain.rs Outdated Show resolved Hide resolved
crates/pop-parachains/src/relay.rs Outdated Show resolved Hide resolved
crates/pop-parachains/src/up/mod.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/up/parachain.rs Outdated Show resolved Hide resolved
crates/pop-cli/src/commands/up/parachain.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@AlexD10S AlexD10S left a comment

Choose a reason for hiding this comment

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

Looks great to me, and I see you’ve addressed all of Peter’s comments.
Thank you for the hard work you’ve put into this!

@evilrobot-01
Copy link
Contributor Author

Looks great to me, and I see you’ve addressed all of Peter’s comments.

Assume they will need resolving before merging. Figure its best to have whomever is merging to resolve to ensure they are happy, rather than me resolving and things get hidden.

@AlexD10S AlexD10S merged commit c1ed1ce into main Dec 19, 2024
19 of 20 checks passed
@AlexD10S AlexD10S deleted the frank/hrmp branch December 19, 2024 09:28
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.

5 participants