-
Notifications
You must be signed in to change notification settings - Fork 482
Update subxt version #1750
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
Conversation
15570c0 to
f60db82
Compare
f60db82 to
66793ad
Compare
crates/e2e/src/client.rs
Outdated
| scale_decode::DecodeAsType, | ||
| scale_encode::EncodeAsType, | ||
| )] | ||
| #[decode_as_type(trait_bounds = "")] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 (?)
There was a problem hiding this comment.
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/src/client.rs
Outdated
| scale_decode::DecodeAsType, | ||
| scale_encode::EncodeAsType, | ||
| )] | ||
| #[decode_as_type(trait_bounds = "")] |
There was a problem hiding this comment.
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?
crates/e2e/src/xts.rs
Outdated
| signer: &Signer<C>, | ||
| ) -> ExtrinsicEvents<C> { | ||
| let call = subxt::tx::StaticTxPayload::new( | ||
| let call = subxt::tx::Payload::new_static( |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to new
crates/e2e/src/xts.rs
Outdated
| gas_limit: Weight, | ||
| storage_deposit_limit: Option<B>, | ||
| value: E::Balance, | ||
| gas_limit: subxt::utils::Static<Weight>, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
ascjones
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
jsdw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
We bump
subxtto0.28.0version. Main changes are due to newscale_en/decodecrates used bysubxt(check paritytech/subxt#842).