Closed serban300 closed 1 month ago
@bkchr @ggwpez this is important and becoming stale, please review
The current implementation is enough for the usage or should we also implement it automatically in derive macro?
EDIT: I feel it shouldn't be difficult, except for skipped
fields, either we can ignore them or we should ask optionally to give the allocation needed for the default field. like #[codec(skipped(allocation = 1000))]
The current implementation is enough for the usage or should we also implement it automatically in derive macro?
EDIT: I feel it shouldn't be difficult, except for
skipped
fields, either we can ignore them or we should ask optionally to give the allocation needed for the default field. like#[codec(skipped(allocation = 1000))]
@gui1117 thank you very much for the review !
Yes, it would be nice to also implement it automatically. Normally it should be an auto trait, but this feature is still unstable so I couldn't use that.
Implementing it automatically in the derive macros would be the next best thing, but I didn't manage to do it. For example for:
struct Test
{
a: u32
}
I tried something that would expand to:
impl DecodeWithMemTracking for Test where u32: DecodeWithMemTracking {
}
Which generates compile errors.
So probably we'll need to implement it manually for each custom struct.
Yes, it would be nice to also implement it automatically. Normally it should be an auto trait, but this feature is still unstable so I couldn't use that.
Implementing it automatically in the derive macros would be the next best thing, but I didn't manage to do it. For example for:
struct Test { a: u32 }
I tried something that would expand to:
impl DecodeWithMemTracking for Test where u32: DecodeWithMemTracking { }
Which generates compile errors.
So probably we'll need to implement it manually for each custom struct.
I see we can't derive it automatically with Decode
derive-macro, but we could provide a DecodeWithMem
derive-macro.
This macro will do the bound same as Encode/Decode, i.e. bound only the generic types. and it will fail if some concrete types don't implement DecodeWithMemTracking
.
It would still be more handy than manually write the code I guess.
I see we can't derive it automatically with
Decode
derive-macro, but we could provide aDecodeWithMem
derive-macro.This macro will do the bound same as Encode/Decode, i.e. bound only the generic types. and it will fail if some concrete types don't implement
DecodeWithMemTracking
.It would still be more handy than manually write the code I guess.
I'm not sure if it's possible. Unless we add a mock method to the DecodeWithMemTracking
trait and try to call it in the derived implementation. Which would be a bit of a hack. But even so, implementing it manually isn't so hard. It's just an empty marker trait. I'm not sure if deriving it would make things much easier.
I see we can't derive it automatically with
Decode
derive-macro, but we could provide aDecodeWithMem
derive-macro. This macro will do the bound same as Encode/Decode, i.e. bound only the generic types. and it will fail if some concrete types don't implementDecodeWithMemTracking
. It would still be more handy than manually write the code I guess.I'm not sure if it's possible. Unless we add a mock method to the
DecodeWithMemTracking
trait and try to call it in the derived implementation. Which would be a bit of a hack. But even so, implementing it manually isn't so hard. It's just an empty marker trait. I'm not sure if deriving it would make things much easier.
I see it now, indeed. :-/
Did you try this will work on Substrate? I mean you will need to ensure that
Call
implementsDecodeWithMemTracking
and all the types we are using there.
Oh, you're right. I can patch polkadot-sdk
locally and try this. Will do it and get back.
Also you should expose a function
decode_with_mem_limit
instead of exposing theMemLimitInput
Yes, will do this too.
I still think requiring to implement it manually can be painful. But if the types requiring this are not many then it good.
Yes, sorry, you're right. We need to be able to derive it since for types like RuntimeCall
we can't implement it manually. Done.
Did you try this will work on Substrate? I mean you will need to ensure that
Call
implementsDecodeWithMemTracking
and all the types we are using there.Oh, you're right. I can patch
polkadot-sdk
locally and try this. Will do it and get back.
Done. This compiles for rococo-runtime
: https://github.com/serban300/polkadot-sdk/tree/mem-limit-decode . It's very verbose but seems to be implemented for all the types that we need. Also had to add logic for deriving DecodeWithMemTracking
.
Also you should expose a function
decode_with_mem_limit
instead of exposing theMemLimitInput
Yes, will do this too.
Done
@bkchr Addressed all the comments. PTAL !
@bkchr just wanted to check if you would like to take another look on the PR or if I could merge it as it is since it already has 2 approving reviews
Related to https://github.com/paritytech/parity-scale-codec/issues/609
Ended up with almost the same approach proposed by @ggwpez in https://github.com/paritytech/parity-scale-codec/pull/602, but made it type safe through a marker trait.
This is not a breaking change.
The inconvenience is that
DecodeWithMemTracking
will need to be implemented manually for each struct.