Decode with mem limit

[serban300]

Related to #609

Ended up with almost the same approach proposed by @ggwpez in #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.

[acatangiu]

@bkchr @ggwpez this is important and becoming stale, please review

[gui1117]

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]

overall looks good to me, some comments.

[serban300]

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.

[gui1117]

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.

[gui1117]

I suggested some more types to implement the trait on.

But part from this it looks good to me.

[serban300]

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'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.

[gui1117]

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'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. :-/

[gui1117]

Looks good to me.

I still think requiring to implement it manually can be painful. But if the types requiring this are not many then it good.

[bkchr]

Did you try this will work on Substrate? I mean you will need to ensure that Call implements DecodeWithMemTracking and all the types we are using there.

Also you should expose a function decode_with_mem_limit instead of exposing the MemLimitInput

[serban300]

Did you try this will work on Substrate? I mean you will need to ensure that Call implements DecodeWithMemTracking 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 the MemLimitInput

Yes, will do this too.

[serban300]

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.

[serban300]

Did you try this will work on Substrate? I mean you will need to ensure that Call implements DecodeWithMemTracking 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 the MemLimitInput

Yes, will do this too.

Done

@bkchr Addressed all the comments. PTAL !

[bkchr]

Some last nitpicks. Otherwise it already looks good :)

[serban300]

@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

[bkchr]

Nice work 👍 Thank you for doing this!