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

Add defaults for pallet-xcm #1959

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

franciscoaguirre
Copy link
Contributor

@franciscoaguirre franciscoaguirre commented Oct 20, 2023

Somewhat related to #2169.
It's not XcmConfig defaults, but pallet-xcm, which is also useful.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

I haven't really seen this, I think it is a great start. I think if you bring it back up to date, it should be almost merge-able.

@gupnik can you provide context on using drive_impl for a trait other than #[pallet::config] trait Config { }. I am sure it is possible, but some guidelines or best practices might be useful.

type CurrencyMatcher: MatchesFungible<BalanceOf<Self>>;

// TODO: Would love to add a default for this, but I need access to `RuntimeOrigin`
// from the derive_impl
#[pallet::no_default]
Copy link
Contributor

Choose a reason for hiding this comment

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

What is usually the default that you would want to provide?

Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to use no_default_bounds for such cases.

LocationWithAssetFilters,
};

pub struct TestDefaultConfig;
Copy link
Contributor

Choose a reason for hiding this comment

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

I belive we should be able to make struct TestDefaultConfig be struct TestDefaultConfig<Some, Further, Parameterization> if you are unsure about any of the things hardcoded in the following parameter_types!.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be extremely helpful, however, I'm getting an error when trying to specify these generics in the calling derive_impl(TestDefaultConfig<Some, Further, Parametrization>). Is this supposed to work or it should be added to the macro?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there is a fundamental blocker for it, but it is quite likely that the syntax is not supported. Please proceed with @gupnik.

@gupnik
Copy link
Contributor

gupnik commented Apr 2, 2024

@gupnik can you provide context on using drive_impl for a trait other than #[pallet::config] trait Config { }. I am sure it is possible, but some guidelines or best practices might be useful.

Yeah, nothing in derive_impl is specific for the pallet config. We already use this for tests. https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/support/test/tests/derive_impl.rs is one such example.

@franciscoaguirre franciscoaguirre changed the title Add defaults for XcmConfig and pallet-xcm Add defaults for pallet-xcm Apr 29, 2024
@bkchr
Copy link
Member

bkchr commented Jul 24, 2024

@franciscoaguirre will you pick this up again?

@franciscoaguirre
Copy link
Contributor Author

I want to but I've been having other priorities

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6842957

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