CodeChain-io / foundry

A programmable open source blockchain engine
GNU General Public License v3.0
38 stars 12 forks source link

Improve RLP macros #99

Open junha1 opened 4 years ago

junha1 commented 4 years ago

Our RLP heavily depends on Ethereum's implementation(https://github.com/paritytech/rlp). They provide some procedural macros that generate encoding/decoding code for basic objects, but it lacks of many details and functionality. Though it helps on some level, we had to manually implement encoding/decoding routines in many cases. I believe that there will be countless new types of data that should be serialized, and we are going to code repetitive functions again and again if there's no some new powerful metaprogramming support.

(see https://github.com/CodeChain-io/foundry/issues/65)

majecty commented 4 years ago

We need some examples or code suggestions to discuss this more.

Could you give some examples of procedural macros that can be used to create serialize/deserialize RLP encoding?

You don't need to give the definition of the procedural macros. I want to see what can be changed if we use Procedural macros.

junha1 commented 4 years ago

@majecty We've already used procedural macros, but the point is that it's limited in very simple cases. For example we can't use it on any struct that has a tuple in it.

majecty commented 4 years ago

Before starting change codes a lot, we need to design and schedule it. We can't discuss from the subject "Let's use procedure macro". Someone should analyze the current status and design on how to change code. Then other people can participate in discussing the subject. So @junha1, could you analyze the current code and suggest how to change them? Then we can discuss the effect of change and how we change it, etc.

junha1 commented 4 years ago

Screenshot from 2020-01-14 12-21-15 These (not all displayed) are either:

  1. Can be directly removed using #[derive]
  2. Can be removed if we solve this issue
  3. Requires very specific optimization or format, so out of this matter.

For example impl Encodable for TrackerInvoices in core/src/blockchain/invoive_db.rs can be removed if we simply add a tuple recognition in RLP macro

That can be done by adding new pattern on this code (see line 122 in https://github.com/CodeChain-io/rlp/blob/master/rlp-derive/src/en.rs)

However it seems to lack priority and quite time consuming, so I'll just close this issue. @majecty @kseo

sgkim126 commented 4 years ago

Using a macro for RLP is a double-edged sword. When you are using macros for RLP you must consider that the order of the items is important in RLP encoding. When you used a macro for some structure instead of implementing it manually, you couldn't change the order of the field.

junha1 commented 4 years ago

We just decided to keep this opened after some discussion with @kseo. It doesn't seem to be important right now but is worth it enough to stay opened and be discussed later.