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 parachain example #1228

Merged
merged 6 commits into from
Oct 30, 2023
Merged

fix parachain example #1228

merged 6 commits into from
Oct 30, 2023

Conversation

Daanvdplas
Copy link
Contributor

The parachain example was not working this fixes it (I took reference from the PolkadotConfig).

I wasn't able to fix the fact that it doesn't run the example from the root folder:
cargo run --bin parachain-example errors:

error: no bin target named parachain-example.

@Daanvdplas Daanvdplas requested a review from a team as a code owner October 27, 2023 11:52
Copy link
Contributor

@tadeohepperle tadeohepperle 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, I missed that, thanks for your PR. :)

Comment on lines 21 to 25
type ExtrinsicParams = <SubstrateConfig as Config>::ExtrinsicParams;
type ExtrinsicParams = StatemintExtriniscParams<Self>;
}

pub type StatemintExtriniscParams<T> = DefaultExtrinsicParams<T>;
pub type StatemintExtrinsicParamsBuilder<T> = DefaultExtrinsicParamsBuilder<T>;
Copy link
Collaborator

@jsdw jsdw Oct 30, 2023

Choose a reason for hiding this comment

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

So previously, we used <SubstrateConfig as Config>::ExtrinsicParams, which resolves to SubstrateExtrinsicParams<SubstrateConfig>, which is DefaultExtrinsicParams<SubstrateConfig>.

With the change, we are using DefaultExtrinsicParams<PolkadotConfig> essentially. Which means that this whole StatemintConfig is now identical to PolkadotConfig.

I thought that the point of the difference was that Statemint used mostly polkadot config but one could tip using assetIDs (which used to be just a substrate config thing). Nowadays, both configs use thesame ExtrinsicParams.

I also wonder; does StateMint want tip asset IDs to be MultiLocations (which we currently don't support in Subxt but will soon, though one can write custom config to support this already).

@Daanvdplas what was not working that this fixed?
And if we want it to be correct, do we need to support MultiLocations in asset IDs too (I think so, but yeah we can do something about that later)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean with "tip".

The error I got:
Screenshot 2023-10-30 at 13 56 42

In addition, in the README.md it says you can run this example by running:
cargo run --bin parachain-example

But this doesn't work. My PR doesn't include the solution for this. Only for running the example successfully.

Copy link
Collaborator

@jsdw jsdw Oct 30, 2023

Choose a reason for hiding this comment

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

Aah ok, I see, thank you!

Since the config is actually identical to PolkadotConfig now, let's simplify first lines of this file to:

use subxt::{
    PolkadotConfig,
    utils::{AccountId32, MultiAddress},
    OnlineClient,
};
use subxt_signer::sr25519::dev::{self};

#[subxt::subxt(runtime_metadata_path = "statemint_metadata.scale")]
pub mod statemint {}

// PolkadotConfig or SubstrateConfig will suffice for this example at the moment,
// but PolkadotConfig is a little more correct, having the right `Address` type.
type StatemintConfig = PolkadotConfig;

#[tokio::main]
pub async fn main() {
 // ....
}

The cargo run --bin parachain-example works if you run it in the same folder as the README, which I think is a reasonably standard assumption, but we could be more explicit in the README about that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cargo run will work too then :). I thought it would be possible to run it from the root of the repo.

I will add your proposed changes!

Copy link
Collaborator

Choose a reason for hiding this comment

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

True; I guess at least the current command makes sure it won't work at all if ran from the wrong folder :D

@jsdw jsdw requested a review from a team as a code owner October 30, 2023 14:56
@tadeohepperle
Copy link
Contributor

@Daanvdplas Do you want to merge it into master? :)

@Daanvdplas Daanvdplas merged commit c3831c2 into master Oct 30, 2023
10 checks passed
@Daanvdplas Daanvdplas deleted the daan/fix-parachain_example branch October 30, 2023 16:08
@tadeohepperle tadeohepperle restored the daan/fix-parachain_example branch October 30, 2023 16:36
@jsdw jsdw mentioned this pull request Dec 7, 2023
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.

None yet

3 participants