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-gateway: Add gateway pallet #1456

Merged
merged 8 commits into from
Jul 18, 2023
Merged

connectors-gateway: Add gateway pallet #1456

merged 8 commits into from
Jul 18, 2023

Conversation

cdamian
Copy link
Contributor

@cdamian cdamian commented Jul 14, 2023 •
edited
Loading

Description

Part 1 of #1376

NOTE - given the dependencies in the original PR, the integration tests will be added in a following PR once all the other parts like the Ethereum Transaction pallet or the Connectors Gateway routers are added.

Changes and Descriptions

  • Add Connectors Gateway pallet.

Checklist:

  • I have added Rust doc comments to structs, enums, traits and functions
  • I have made corresponding changes to the documentation
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works

@cdamian cdamian mentioned this pull request Jul 14, 2023
Closed
4 tasks
lemunozm
lemunozm reviewed Jul 17, 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.

First of all, thanks for splitting the PR!

The review was quite easy thanks that everything is very well organized & clean @cdamian! 🎉 Left some comments

libs/mocks/src/connectors_gateway_routers.rs Outdated Show resolved Hide resolved
pallets/connectors-gateway/src/lib.rs Outdated Show resolved Hide resolved
pallets/connectors-gateway/src/lib.rs Show resolved Hide resolved
pallets/connectors-gateway/src/mock.rs Outdated Show resolved Hide resolved
pallets/connectors-gateway/Cargo.toml Outdated Show resolved Hide resolved
pallets/connectors-gateway/src/origin.rs Show resolved Hide resolved
pallets/connectors-gateway/src/tests.rs Outdated Show resolved Hide resolved
pallets/connectors-gateway/src/tests.rs Outdated Show resolved Hide resolved
pallets/connectors-gateway/src/tests.rs Show resolved Hide resolved
runtime/common/Cargo.toml Show resolved Hide resolved
wischli
wischli previously approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@wischli wischli left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I love how well structured and clean your work is! Thanks a lot for splitting the PR which made this review much easier! 🫶

Apart from a few praising comments, I only have a few nits which could also be ignored.

pallets/connectors-gateway/src/lib.rs Show resolved Hide resolved
runtime/development/src/connectors.rs Show resolved Hide resolved
runtime/development/src/connectors.rs

parameter_types! {
// TODO(cdamian): Double-check these.
pub const MaxIncomingMessageSize: u32 = 1024;
Copy link
Contributor

Choose a reason for hiding this comment

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

How can this value be verified? Max PoV size is 5 * 1024 * 1024.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that we can have something similar to MaxEncodedLen for this purpose. Then, whatever implements the (Connectors) Codec trait, would have a known max size when encoded. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Implicitly, we already have a MaxEncodedLen for Connectors messages since we have our custom Codec implementation. Of course, we are missing the trait implementation at this point.

AFAICS, the max encoded messages would be ExecutedCollectRedeem and ExecutedCollectInvest with 8+16+32+16+16+16+16 = 120 bytes. However, these should only be outgoing. For incoming, the largest should be TransferTrancheTokens with 88 bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As agreed, let's stick to the hard-coded value for the time being. Should I create a follow-up issue for this so we can investigate at a later time?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should have a MaxEncodedLen impl for the types implementing connectors::Codec

pallets/connectors-gateway/src/lib.rs Outdated Show resolved Hide resolved
pallets/connectors-gateway/src/tests.rs Outdated Show resolved Hide resolved
pallets/connectors-gateway/src/tests.rs Outdated Show resolved Hide resolved
pallets/connectors-gateway/src/tests.rs Outdated Show resolved Hide resolved
pallets/connectors-gateway/src/tests.rs Outdated Show resolved Hide resolved
pallets/connectors-gateway/src/tests.rs Outdated Show resolved Hide resolved
pallets/connectors-gateway/src/lib.rs Outdated Show resolved Hide resolved
@cdamian cdamian force-pushed the connectors-gateway-1-gateway-pallet branch from f4872c8 to f1ed2f3 Compare July 17, 2023 09:46
@lemunozm
Copy link
Contributor

@cdamian, I've seen you have answered all comments except two that are just hidden because Github hides some in large conversations. Only double-check that you know about them.

@cdamian
Copy link
Contributor Author

cdamian commented Jul 17, 2023

@cdamian, I've seen you have answered all comments except two that are just hidden because Github hides some in large conversations. Only double-check that you know about them.

I always forget about those hidden comments... should all be addressed by now.

lemunozm
lemunozm previously approved these changes Jul 18, 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! Really like it!

@cdamian cdamian force-pushed the connectors-gateway-1-gateway-pallet branch from 5acba49 to 1fd8c5c Compare July 18, 2023 09:57
@cdamian cdamian mentioned this pull request Jul 18, 2023
Open
@cdamian cdamian force-pushed the connectors-gateway-1-gateway-pallet branch from 1fd8c5c to 2d6056c Compare July 18, 2023 14:52
wischli
wischli approved these changes Jul 18, 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! Thanks for applying all the changes 🚀

@cdamian cdamian enabled auto-merge (squash) July 18, 2023 15:05
mikiquantum
mikiquantum approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@mikiquantum mikiquantum left a comment

Choose a reason for hiding this comment

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

LGTM

pallets/connectors-gateway/src/lib.rs
) -> DispatchResult {
T::AdminOrigin::ensure_origin(origin)?;

ensure!(domain != Domain::Centrifuge, Error::<T>::DomainNotSupported);
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we will remove this check as soon as we wrap up subsequent PRs right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will stay, we can only remove this if we do some refactoring that gives us an associated type that we know is not a Centrifuge domain.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We could have

type Domain: Member + Parameter;

type LocalDomain: Get<Domain>;

@mikiquantum mikiquantum disabled auto-merge July 18, 2023 18:11
@mikiquantum mikiquantum merged commit 4dc6d26 into main Jul 18, 2023
mustermeiszer
mustermeiszer reviewed Aug 9, 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.

Nothing relevant.

pallets/connectors-gateway/src/lib.rs
) -> DispatchResult {
T::AdminOrigin::ensure_origin(origin)?;

ensure!(domain != Domain::Centrifuge, Error::<T>::DomainNotSupported);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could have

type Domain: Member + Parameter;

type LocalDomain: Get<Domain>;

runtime/development/src/connectors.rs

parameter_types! {
// TODO(cdamian): Double-check these.
pub const MaxIncomingMessageSize: u32 = 1024;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should have a MaxEncodedLen impl for the types implementing connectors::Codec

@cdamian cdamian mentioned this pull request Aug 9, 2023
Open
@NunoAlexandre NunoAlexandre deleted the connectors-gateway-1-gateway-pallet branch September 4, 2023 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mustermeiszer mustermeiszer mustermeiszer left review comments

@wischli wischli wischli approved these changes

@lemunozm lemunozm lemunozm approved these changes

@mikiquantum mikiquantum mikiquantum approved these changes

@branan branan Awaiting requested review from branan

@NunoAlexandre NunoAlexandre Awaiting requested review from NunoAlexandre

@hieronx hieronx Awaiting requested review from hieronx hieronx is a code owner

@thea-leake thea-leake Awaiting requested review from thea-leake

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cdamian @lemunozm @mikiquantum @wischli @mustermeiszer