LP Message with custom data format for serialization/deserialization

[lemunozm]

Description

Current serialization/deserialization methods don't scale well and are error-prone. This PR adds a custom serde data format that implements our liquidity pool's Generic Message Passing Format. This would remove entirely the need for manual serialization/deserialization code

[lemunozm]

Currently, the serialization is the same except for the 0 padding used for enum variants and the Domain type. I think both can be tricked to fit the standard.

[wischli]

I wish this PR existed 24 hours ago when I decided to start the message refactoring while waiting for compilers 🥲

Would be awesome if this worked out!

[lemunozm]

I think once we get the correct serialization, it can be easily ported to your branch.

[lemunozm]

Both issues are now fixed and the serialization & deserialization is the same as expected

[mustermeiszer]

Not sure I like the change. The raw types look weird, but the spec itself matches the code now better.

No strong opinion as long as it is compatible. The main issue for me is that we do not control the serialization here. But tests should cover everything.

[mustermeiszer]

THen lets go with this approach.

[wischli]

As I am currently working on the message reordering, I have added all known fields for the new messages already. In order to not do redundant work, I would like this PR to get merged ASAP. However, it should be based on the new LP v2 feature branch feat/lp-v2 which I just branched off from main.

It appears to me that we have found consensus and can finish this PR today @lemunozm @mustermeiszer?

[lemunozm]

Why not leave this directly in main? as this does not add anything LPv2 related

[lemunozm]

And rebase later LPv2 over it. I understand that LPv2 is a branch that should be rebased over main continuously, right?

[wischli]

Why not leave this directly in main? as this does not add anything LPv2 related

Also fine with that!

[wischli]

I understand that LPv2 is a branch that should be rebased over main continuously, right?

Yes

[lemunozm]

I'm on the way to clean this up and have it ready.

[lemunozm]

Ok, after some thoughts, I think you need to implement custom serialization for the InnerMessage, to be able to force the enum variant as u8

struct Message {
    UpdateRestriction(UpdateRestriction),
}

enum UpdateRestriction {
    UpdateMember {},
    Freeze {},
    Unfreeze {},
}

impl Serialize for UpdateRestriction {
    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
         match self {
             UpdateMember {} => {
                 serializer.serialize_u8(1);
                 serializer.serialize_field(..)
             },
             Freeze {} => {
                 serializer.serialize_u8(2);
                 serializer.serialize_field(..)
             },
             ..
         }
    }
}

impl<'de> Deserialize<'de> for UpdateRestriction {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: Deserializer<'de>,
    {
        match deserializer.deserialize_u8(u8Visitor) {
            1 => //Deserialize UpdateMember fields
            2 => //Deserialize Freeze,
            ..
        }
    }
}

[wischli]

Ok, after some thoughts, I think you need to implement custom serialization for the InnerMessage, to be able to force the enum variant as u8
[...]

That's similar to what I had in mind with my limited understanding of the bincode changes impact. I suppose I should try this out on a fork of my lpv2/message-reorder branch rebased to this exp/lp-message-bincode branch?!

[lemunozm]

I guess so. Or maybe merge this today and then rebase the whole feat/lpv2 branch with all sub-branches. Let's see if I can solve the CI issues from this

[wischli]

I guess so. Or maybe merge this today and then rebase the whole feat/lpv2 branch with all sub-branches. Let's see if I can solve the CI issues from this

That would be better overall of course. The lpv2 branch is in sync with main right now btw.

[lemunozm]

I'm not getting to overpass the CI issue. It seems like when bincode has not std enable it still imports something from std 😭. Maybe we should skip this PR until the bincode v2.0.0 formal release 😢. I do not want to block your work.

[lemunozm]

If I make:

cargo clean
cargo build -p centrifuge-chain -r

I obtain the CI issue locally.

If I do:

// Comment bincode in cargo.toml
cargo clean
cargo build -p centrifuge-chain -r
// Uncomment bincode in Cargo.toml
cargo build -p centrifuge-chain -r

It works 😅

[lemunozm]

I couldn't bypass the CI issue because something is off with bincode and no_std.

So, I've decided to implement a custom serializer. It has been simpler than I expected (around 500 lines in total, quite straightforward). I think it's the purest and most scalable solution, allowing us to define the GMPF encoding correctly and clean the message module from handling bytes directly, so I would go with it.

cc @mustermeiszer

[codecov]

Codecov Report

Attention: Patch coverage is 29.53020% with 210 lines in your changes missing coverage. Please review.

Project coverage is 46.03%. Comparing base (2f08ae7) to head (1930137).

Files	Patch %	Lines
pallets/liquidity-pools/src/gmpf/ser.rs	33.03%	75 Missing ⚠️
pallets/liquidity-pools/src/gmpf/de.rs	34.83%	58 Missing ⚠️
pallets/liquidity-pools/src/lib.rs	22.00%	39 Missing ⚠️
pallets/liquidity-pools/src/hooks.rs	0.00%	14 Missing ⚠️
pallets/liquidity-pools/src/gmpf/error.rs	0.00%	12 Missing ⚠️
pallets/liquidity-pools/src/message.rs	33.33%	8 Missing ⚠️
pallets/liquidity-pools/src/inbound.rs	0.00%	3 Missing ⚠️
libs/traits/src/liquidity_pools.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1889      +/-   ##
==========================================
- Coverage   47.38%   46.03%   -1.35%     
==========================================
  Files         176      179       +3     
  Lines       13305    13093     -212     
==========================================
- Hits         6304     6028     -276     
- Misses       7001     7065      +64     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lemunozm]

@wischli this should be ready to unblock your work on the message reorder

[mustermeiszer]

Why do we need to always call gmpf::to_vec() for a message and not just serialize()?

[lemunozm]

Why do we need to always call gmpf::to_vec() for a message and not just serialize()?

Both are the same, actually serialize() calls gmpf::to_vec() but without failing (empty vec if it fails because you do not expect to fail in production when serializing). Nevertheless, I've used directly gmpf::to_vec() in the tests because I want to know the explict error if it can not be serialized well. So, tests can fail with an error, but production should be infallible if tests pass.

Outside of that module, you can/should just call to .serialize() as currently happens in the gateway.

[wischli]

I'm honestly surprised to see that implementing the byte serializer suffices even though it is missing for the deserializer. Thanks for putting so much work into this! From my perspective, we can merge this but I'd like to wait for @mustermeiszer's opinion.

[wischli]

Am I missing something or should this be implemented before we merge this PR?

[lemunozm]

Good question! I was surprised too when implemented this. It results that the fixed arrays are serialized/deserialized as tuples (if you think about it, actually it makes sense). Only dynamic lists of bytes are deserialized with such method, and we do not have such fields right now

[wischli]

Just to be sure: The call type is now inherited from the order of the enum variants, right? So when I have a nested enum Y inside an LP Message X, Y will have its own inner call type such that X has an outer and an inner one.

[lemunozm]

The call type is now inherited from the order of the enum variants, right?

Yes

So when I have a nested enum Y inside an LP Message X, Y will have its own inner call type such that X has an outer and an inner one.

And yes too

[lemunozm]

Probably we should have a test checking that each msg has the expected index in the spec, to avoid errors swapping variants in the future.

[wischli]

Yeah I was thinking of exactly that. It is more or less tested in the message serialization cycle UTs in Message.rs but these are too opaque to guarantee call type safety. No need for you to do it as part of this PR because I am changing almost all indices anyway. Will do it as part of #1892 then.

[lemunozm]

It is more or less tested in the message serialization cycle UTs in Message.rs but these are too opaque to guarantee call type safety

fully agree!

[lemunozm]

To avoid boilerplate in testing, maybe each test construction can go to a function: i.e. increase_invest_order_msg(), and later:

`assert_eq!(msg_increase_invest_order().serialize()[0], 5)`

or similar.

Also, not sure if the it's more manageable to have the message as:

enum Message {
    AddPool(AddPoolMessage),
    AllowInvestCurrency(AllowInvestCurrencyMessage),
    ..
},

It seems like "more" code at first, but later, you can reuse each variant for different purposes. I'm just thinking out loud.

[wischli]

Also, not sure if the it's more manageable to have the message as:

enum Message {
    AddPool(AddPoolMessage),
    AllowInvestCurrency(AllowInvestCurrencyMessage),
    ..
},

Not sure if I see the win for doing it this way right now.

[wischli]

LGTM!

[mustermeiszer]

Enum call type encoding should be checked by integration tests implicitly. If we cover all messages in the next month.