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

Connectors v1++ #1211

Merged
merged 38 commits into from Mar 3, 2023
Merged

Connectors v1++ #1211

merged 38 commits into from Mar 3, 2023

Conversation

NunoAlexandre
Copy link
Contributor

@NunoAlexandre NunoAlexandre commented Feb 22, 2023 •
edited

Changes

  • feat: Add the ability to receive Connector Messages from the XcmRouter contract on Moonbase
  • Implement Decode for Message
  • Extend XCM config for development
    • handle incoming Transact messages with DescendOrigin instruction at head
    • Support arbitrary MultiLocation to AccountId conversion (AccountIdHash)
  • Improve and extend tests message encoding/decoding
  • Improve / clean up message encoding code
  • Move util functions to cfg-utils
  • Bump development spec_version

To Do

  • Add origin check for the origin of the handle function
  • Implement custom Decode for Message
  • Test the decoding implementation
  • 🔴 Test everything one last time before merging

@NunoAlexandre NunoAlexandre self-assigned this Feb 23, 2023
NunoAlexandre
NunoAlexandre commented Feb 24, 2023
View reviewed changes
Copy link
Contributor Author

@NunoAlexandre NunoAlexandre left a comment

Choose a reason for hiding this comment

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

Some notes on the Xcm config changes 👀

runtime/development/src/xcm.rs Outdated Show resolved Hide resolved
runtime/development/src/xcm.rs Outdated Show resolved Hide resolved
@NunoAlexandre NunoAlexandre mentioned this pull request Feb 24, 2023
Open
@NunoAlexandre NunoAlexandre marked this pull request as ready for review February 24, 2023 10:54
lemunozm
lemunozm previously approved these changes Feb 24, 2023
View reviewed changes
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

LGTM! Simple, and readable! A lot of cool factorizations 🚀 .

Minor comments below

libs/utils/src/lib.rs Outdated Show resolved Hide resolved
pallets/connectors/src/lib.rs Outdated Show resolved Hide resolved
pallets/connectors/src/message.rs Outdated Show resolved Hide resolved
pallets/connectors/src/message.rs Outdated Show resolved Hide resolved
pallets/connectors/src/message.rs Outdated Show resolved Hide resolved
pallets/connectors/src/message.rs Show resolved Hide resolved
wischli
wischli previously approved these changes Feb 24, 2023
View reviewed changes
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

LGTM, great work! I also agree with @lemunozm's minor comments.

pallets/connectors/src/lib.rs Show resolved Hide resolved
mustermeiszer
mustermeiszer previously approved these changes Feb 24, 2023
View reviewed changes
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

Looks already good!

Just some minor stuff that could be adapted.

pallets/connectors/src/message.rs Outdated Show resolved Hide resolved
pallets/connectors/src/message.rs Show resolved Hide resolved
pallets/connectors/src/message.rs Show resolved Hide resolved
lemunozm
lemunozm reviewed Feb 28, 2023
View reviewed changes
pallets/connectors/src/message.rs
fn add_pool_zero() {
let msg = Message::<Domain, PoolId, TrancheId, Balance, Rate>::AddPool { pool_id: 0 };
let encoded = msg.encode();
pub type ConnectorMessage = Message<Domain, PoolId, TrancheId, Balance, Rate>;
Copy link
Contributor

@lemunozm lemunozm Feb 28, 2023 •
edited

Choose a reason for hiding this comment

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

I'm worried that because Message is something generic for these types, we could be testing that it's working with the test types, but different configurations of the connector pallet could lead to different serializations. i.e. How a Rate is encoding is defined by the Rate used in the configuration. Nevertheless is the pallet who should set how that rate should be deserialized by the data format specification agreed upon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's a good point. What safe-guards us is the expected_hex values, that's why it's important to have those in the tests 👍

wischli
wischli previously approved these changes Feb 28, 2023
View reviewed changes
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

LGTM!

mustermeiszer
mustermeiszer previously approved these changes Feb 28, 2023
View reviewed changes
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment •
edited

Choose a reason for hiding this comment

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

We went for Option<B> right?

I think it is fine to move forward with this, knowing that we need to adapt the encoding/decoding anyways because of V2 in the near future. We should revisit the proposal then again and have a stable, secure way of doing that on substrate and solidity then.

@NunoAlexandre
Copy link
Contributor Author

We went for Option<B> right?

I think it is fine to move forward with this, knowing that we need to adapt the encoding/decoding anyways because of V2 in the near future. We should revisit the proposal then again and have a stable, secure way of doing that on substrate and solidity then.

Yes, sorry, I messed up A and B. Still need to learn the letters apparently 😄

And agreed on the approach 💯

lemunozm
lemunozm previously approved these changes Mar 2, 2023
View reviewed changes
Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

LGTM!

mustermeiszer
mustermeiszer previously approved these changes Mar 2, 2023
View reviewed changes
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

Re-Approve.

@NunoAlexandre NunoAlexandre enabled auto-merge (squash) March 3, 2023 10:47
mustermeiszer
mustermeiszer approved these changes Mar 3, 2023
View reviewed changes
Copy link
Collaborator

@mustermeiszer mustermeiszer left a comment

Choose a reason for hiding this comment

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

Re-approve

@NunoAlexandre NunoAlexandre merged commit d1f436e into main Mar 3, 2023
wischli
wischli reviewed Mar 3, 2023
View reviewed changes
Copy link
Contributor

@wischli wischli left a comment

Choose a reason for hiding this comment

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

Re-approving!

@NunoAlexandre NunoAlexandre deleted the connectors-v1++ branch March 3, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wischli wischli wischli left review comments

@mustermeiszer mustermeiszer mustermeiszer approved these changes

@lemunozm lemunozm lemunozm left review comments

@mikiquantum mikiquantum Awaiting requested review from mikiquantum

Assignees

@NunoAlexandre NunoAlexandre

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@NunoAlexandre @mikiquantum @lemunozm @wischli @mustermeiszer