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.

Sign up for GitHub

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

Jump to bottom

Feature/plmc 287 parachain information discovery and channel opening #94

Merged
merged 4 commits into from
Nov 7, 2023
Merged

Feature/plmc 287 parachain information discovery and channel opening #94

merged 4 commits into from
Nov 7, 2023

Conversation

JuaniRios
Copy link
Contributor

@JuaniRios JuaniRios commented Oct 23, 2023 •
edited
Loading

PLMC 287: Parachain information discovery and channel opening

Objective

Add a way for an issuer to add information about its parachain when it launches, and use that to automatically accept their hrmp request (based on certain criteria), and also send an hrmp request to them.

Features

  • New chain spec called "testing-node" which is only available when compiling with the feature of the same name.

    • We can now use the instantiator to create projects inside the genesis config (only with testing-node feature)
    • We can use the instantiator parameter generation functions (like generating bids) inside a chain spec definition, or just use a json file with the raw values

  • New fields to represent the paraId and the status of the channels.

    • ParaId set by an extrinsic
    • Channel status automatically set

  • Fork of XcmExecutor to add our own implementation of the HRMP instructions

    • The original executor returned an error. We now use those instructions for automatically setting the channels with the relay
    • We keep all the traits coming from the original executor, and only provide our own XcmExecutor struct

@linear
Copy link

linear bot commented Oct 23, 2023

PLMC-287 Parachain information discovery and channel opening

@JuaniRios JuaniRios force-pushed the feature/plmc-287-parachain-information-discovery-and-channel-opening branch 4 times, most recently from 13ad46e to 3ed35dd Compare October 26, 2023 10:44
@JuaniRios
Copy link
Contributor Author

JuaniRios commented Oct 26, 2023 •
edited
Loading

PR is ready for review.
To test the main feature of this branch, check the following doc:
How to test HRMP channel connection.pdf

@JuaniRios JuaniRios self-assigned this Oct 26, 2023
@JuaniRios JuaniRios marked this pull request as ready for review October 26, 2023 13:01
@JuaniRios JuaniRios force-pushed the feature/plmc-287-parachain-information-discovery-and-channel-opening branch from 0f42df5 to e7cf200 Compare October 26, 2023 13:02
lrazovic
lrazovic requested changes Oct 30, 2023
View reviewed changes
Copy link
Member

@lrazovic lrazovic left a comment

Choose a reason for hiding this comment

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

One "small" step for Contribution Token Migration, one big step for Polimec.

First Round of Feedback:

I really like how we can now instantiate different projects in different rounds effectively. The updated logic for managing and initiating HRMP channels is also really nice.

While my comments address relatively minor points, I'd appreciate a second perspective from @vstam1, particularly on changes related to XCM aspects.

Cargo.toml Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
integration-tests/src/tests/hrmp_flow.rs Outdated Show resolved Hide resolved
pallets/funding/src/types.rs Outdated Show resolved Hide resolved
pallets/funding/src/types.rs Outdated Show resolved Hide resolved
pallets/funding/src/lib.rs Outdated Show resolved Hide resolved
pallets/funding/src/lib.rs Outdated Show resolved Hide resolved
pallets/xcm-executor/src/config.rs Show resolved Hide resolved
pallets/funding/src/instantiator.rs Outdated Show resolved Hide resolved
pallets/funding/src/instantiator.rs Outdated Show resolved Hide resolved
@lrazovic
Copy link
Member

Also, probably a rebase is needed after #93

vstam1
vstam1 requested changes Oct 30, 2023
View reviewed changes
Copy link
Collaborator

@vstam1 vstam1 left a comment

Choose a reason for hiding this comment

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

Great Initial start for the parachain channel opening. I left a couple of comments, together with the comments from Leonardo.

I'm still a bit concerned about the maintainability of the custom xcm-executor. It is definitely something that we should think about, as parity has common updates to the executor code (I personally worked on the transactionality of xcm instructions. The pr has big changes to the xcm-executor code). So we should probably patch our executor on every polkadot-sdk release.

I think it would also be nice if we could move the migration logic to another pallet to decrease the complexity/lines of code in pallet-funding. Let me know what you guys think!

runtimes/testnet/src/xcm_config.rs Outdated Show resolved Hide resolved
pallets/xcm-executor/src/config.rs Outdated Show resolved Hide resolved
pallets/funding/src/lib.rs Show resolved Hide resolved
pallets/funding/src/functions.rs Show resolved Hide resolved
@JuaniRios JuaniRios force-pushed the feature/plmc-287-parachain-information-discovery-and-channel-opening branch 2 times, most recently from dd31274 to 0e1011a Compare November 6, 2023 10:12
@JuaniRios
feat(287): parachain information discovery and channel opening
fe40084 
fix

migration readiness check

cargo lock

channel open function now included in trait

readme on executor fork

small fixes (Leo's feedback)

save

automatic hrmp connection working

Genesis instantiator usage and first draft of HRMP connection

formatting

feature propagation

cleanup

new node functioning. genesis not yet sure if working

new node functioning. genesis not yet sure if working

somehow compiling and test passing

save

save

save

feat(287): para_id setting extrinsic implemented and tested

save

same log crate across workspace

same log crate across workspace

save

save

save

feat(287): changed tight couple of pallet_xcm by extracting sender trait

feat(287): first commit

feat(285): POC Hrmp automatic acceptance
@JuaniRios JuaniRios force-pushed the feature/plmc-287-parachain-information-discovery-and-channel-opening branch from 6a2c7da to fe40084 Compare November 6, 2023 10:28
@JuaniRios
Copy link
Contributor Author

/bot test pallet-funding

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 6, 2023

test: Succeeded! ✅
https://github.com/Polimec/polimec-node/actions/runs/6769513240

@Polimec Polimec deleted a comment from github-actions bot Nov 6, 2023
@Polimec Polimec deleted a comment from github-actions bot Nov 6, 2023
@Polimec Polimec deleted a comment from github-actions bot Nov 6, 2023
@Polimec Polimec deleted a comment from github-actions bot Nov 6, 2023
@Polimec Polimec deleted a comment from JuaniRios Nov 6, 2023
@Polimec Polimec deleted a comment from JuaniRios Nov 6, 2023
@Polimec Polimec deleted a comment from github-actions bot Nov 6, 2023
vstam1
vstam1 approved these changes Nov 6, 2023
View reviewed changes
Copy link
Collaborator

@vstam1 vstam1 left a comment

Choose a reason for hiding this comment

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

One comment on the robustness of the channel opening. I don't see it as a blocker but would probably be good to implement.

pallets/funding/src/lib.rs Show resolved Hide resolved
@JuaniRios JuaniRios merged commit b8e6c62 into main Nov 7, 2023
@JuaniRios JuaniRios deleted the feature/plmc-287-parachain-information-discovery-and-channel-opening branch November 7, 2023 10:31
@JuaniRios JuaniRios mentioned this pull request Nov 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstam1 vstam1 vstam1 approved these changes

@lrazovic lrazovic lrazovic approved these changes

Assignees

@JuaniRios JuaniRios

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JuaniRios @lrazovic @vstam1