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

Update subxt version #1750

Merged
merged 14 commits into from
Apr 17, 2023
Merged

Conversation

pmikolajczyk41
Copy link
Member

@pmikolajczyk41 pmikolajczyk41 commented Apr 13, 2023

We bump subxt to 0.28.0 version. Main changes are due to new scale_en/decode crates used by subxt (check paritytech/subxt#842).

@pmikolajczyk41 pmikolajczyk41 changed the title [WIP] Update subxt version Update subxt version Apr 13, 2023
scale_decode::DecodeAsType,
scale_encode::EncodeAsType,
)]
#[decode_as_type(trait_bounds = "")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this if trait bounds are empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

because automatically compiler will require bounds for E (in general, for every generic parameter)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps to avoid having this attribute everywhere we could have a blanket impl of DecodeAsType for the Environment trait?

Copy link
Member Author

Choose a reason for hiding this comment

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

we could, but then I would have to provide something reasonable here:

impl DecodeAsType for E {
    fn decode_as_type(input: &mut &[u8], type_id: u32, types: &PortableRegistry) -> Result<Self, Error> {
        todo!()
    }
}

impl EncodeAsType for E {
    fn encode_as_type_to(&self, type_id: u32, types: &PortableRegistry, out: &mut Vec<u8>) -> Result<(), scale_encode::Error> {
        todo!()
    }
}

@jsdw what makes sense here?

Copy link

Choose a reason for hiding this comment

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

Assuming Environment is a type that can't be constructed (eg enum Environment {}) I think some like that would be fine (though I'd panic!("Shouldn't call x on Environment") on the DecodeAsType and maybe unreachable!() on the EncodeAsType one even so).

If it's at all possible to construct an Environment then personally I'd go for the trait bounds everywhere.

(Side note: I'm noting that Encode and Decode don't require such trait bounds; I wonder how that is possible because maybe we can be smarter with EncodeAsType and DecodeAsType in this regard also)

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm... it's a trait and it feels kinda weird to have these panics in the blanket implementation :| I don't have strong opinion; @ascjones (?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a good idea. Just leave as is with the #[decode_as_type(trait_bounds = "")] for now

crates/e2e/Cargo.toml Outdated Show resolved Hide resolved
crates/e2e/src/xts.rs Outdated Show resolved Hide resolved
crates/e2e/src/xts.rs Outdated Show resolved Hide resolved
crates/env/Cargo.toml Show resolved Hide resolved
scale_decode::DecodeAsType,
scale_encode::EncodeAsType,
)]
#[decode_as_type(trait_bounds = "")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps to avoid having this attribute everywhere we could have a blanket impl of DecodeAsType for the Environment trait?

value: E::Balance,
gas_limit: Weight,
storage_deposit_limit: Option<E::Balance>,
data: Vec<u8>,
signer: &Signer<C>,
) -> ExtrinsicEvents<C> {
let call = subxt::tx::StaticTxPayload::new(
let call = subxt::tx::Payload::new_static(
Copy link

Choose a reason for hiding this comment

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

You could also use Payload::new() to use the documented interface (though downside is it would allocate since it doesn't specialise on static strings)

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to new

gas_limit: Weight,
storage_deposit_limit: Option<B>,
value: E::Balance,
gas_limit: subxt::utils::Static<Weight>,
Copy link

Choose a reason for hiding this comment

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

Weight is a super simple struct, so for a little added robustness/simplicity I wonder whether it's worth just writing your own version that impls EncodeAsType et al

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, but sp_weight::Weight comes with pretty broad API and I don't know whether it would be convenient in the end to copy just the struct (copying the whole API seems to be an overkill); I'm leaving the decision to one of this repo maintainers @ascjones ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with a local copy of the type with a minimal API, it is likely to be highly stable.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

crates/env/src/types.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

Nice!

@ascjones ascjones merged commit 644c547 into use-ink:master Apr 17, 2023
@pmikolajczyk41 pmikolajczyk41 deleted the pmikolajczyk41/bump-subxt branch April 17, 2023 13:19
@cmichi cmichi mentioned this pull request Apr 20, 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.

4 participants