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 the Header::Hash type param to the metadata #2178

Closed
wants to merge 2 commits into from

Conversation

serban300
Copy link
Contributor

We need this field to be in the metadata for the generated runtimes that we use for the bridges.

More details:
paritytech/parity-bridges-common#2669
paritytech/subxt#1247

@serban300 serban300 added the T15-bridges This PR/Issue is related to bridges. label Nov 6, 2023
@serban300 serban300 requested a review from a team November 6, 2023 15:17
@serban300 serban300 self-assigned this Nov 6, 2023
@serban300 serban300 requested a review from a team November 6, 2023 15:17
Copy link
Contributor

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

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

👍 for us it is fine, but I wonder if it may break something else, so I'd wait for approval from those who are working on that code

@serban300
Copy link
Contributor Author

@gupnik Could you please take a look on this PR to see if it's ok ? It's a very small change. From what I understand from the git history, I think that you added the #[scale_info(skip_type_params(Hash))] line and this PR removes it.

@bkchr
Copy link
Member

bkchr commented Nov 6, 2023

I think having the skip there is more correct. You are abusing there with subxt the fact that the runtime and subxt is using Rust. Other languages can not just include these types by path. This type is also not really part of the of the type, it is the actual hashing that is being used.

@serban300
Copy link
Contributor Author

serban300 commented Nov 6, 2023

I think having the skip there is more correct. You are abusing there with subxt the fact that the runtime and subxt is using Rust. Other languages can not just include these types by path. This type is also not really part of the of the type, it is the actual hashing that is being used.

I agree that having the skip there is more correct, but so far I couldn't find a good workaround that would enable us to keep generating the runtime apis if we keep the skip. So I don't know what would be the best solution. Still thinking of workarounds on our side.

Converted the PR to draft in the meanwhile.

@bkchr
Copy link
Member

bkchr commented Nov 6, 2023

It gets even weirder if you look into the Config that you need to provide to subxt. It already includes the Hasher and the Header, both of the types we are speaking here about.

CC @jsdw

@serban300 serban300 marked this pull request as draft November 6, 2023 16:45
@serban300
Copy link
Contributor Author

It gets even weirder if you look into the Config that you need to provide to subxt. It already includes the Hasher and the Header, both of the types we are speaking here about.

CC @jsdw

I am not very familiar with this, but I guess that config needs to be provided only if someone uses subxt for submitting the extrinsics. We only use subxt-codegen to generate the runtime api and we have our own code for building and submitting the extrinsics. Also I guess the hasher and the header in that config are the ones of the current chain. For our extrinsics it's the header and hash of the bridged chains, so it gets a bit more complicated.

But for someone that submits extrinsics using subxt, I think that config makes sense

@bkchr
Copy link
Member

bkchr commented Nov 6, 2023

We only use subxt-codegen to generate the runtime api and we have our own code for building and submitting the extrinsics

I mean you are using the metadata of the target chain or? I mean I also have no real idea how subxt works, but @jsdw should be able to help here :P

@serban300
Copy link
Contributor Author

I mean you are using the metadata of the target chain or?

Yes, we're only using the metadata of the target chain to get the definition of the Runtime Call. And with that we can build and submit the calls that we need from the relayers.

@serban300
Copy link
Contributor Author

Closing this PR. I will think of alternatives.

@serban300 serban300 closed this Nov 7, 2023
@jsdw
Copy link
Contributor

jsdw commented Nov 7, 2023

I left a response in the subxt issue for this (paritytech/subxt#1247).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T15-bridges This PR/Issue is related to bridges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants