Merkleized metadata - Githubissues

xlc commented 4 months ago

Maybe we can have a reference test section with some test data? We already have PoC so it shouldn't be hard to make some just by taking from existing tests.

bkchr commented 4 months ago

Maybe we can have a reference test section with some test data? We already have PoC so it shouldn't be hard to make some just by taking from existing tests.

Can you clarify what you mean by test data? What should this section show?

xlc commented 4 months ago

A random example: https://eips.ethereum.org/EIPS/eip-145#test-cases

In this case, I am expecting something like

Input: An example metadata JSON Output: A digest hash

bkchr commented 4 months ago

Input: An example metadata JSON

The metadata is multiple 100KB's big. I don't want to paste this into the document :see_no_evil: I mean I see what you want, but I don't really see on what this brings us here?

josepot commented 4 months ago

I have a couple of questions:

1) In the recent metadata versions (at least in versions 14 and 15), the BitSequence definition looks like this:

struct BitSequence {
    bit_store_type: TypeRef,
    bit_order_type: TypeRef,
}

Which I personally find it quite annoying because the TypeRef of the bit_order_type is always an empty composite, and the only way to infer the least_significant_bit_first field is to look into the path property of said reference, which is quite brittle.

The proposed BitSequence definition of this spec is a lot nicer:

struct BitSequence {
    num_bytes: u8,
    least_significant_bit_first: bool,
}

However, unless I'm mistaken, this change won't make it possible to generate the MetadataDigest using "old" versions of the metadata like v14 and v15, correct?

2) MetadataDigest has references to the fields: token_symbol and decimals. However, AFAIK those fields are currently not being stored on-chain. So, I'm guessing that this RFC implies that these fields will be stored on-chain moving forward?

If that's the case, wouldn't it be beneficial to also mention in the spec what's the canonical way for accessing those field values?

Furthermore, it would probably be a good idea to state what the canonical way is for accessing the values for the fields spec_name, spec_symbol and base58_prefix, which IIRC they are today present in the constants of the "System" pallet. I'm not sure what the right way to reflect that in the spec is, TBH.

bkchr commented 4 months ago

However, unless I'm mistaken, this change won't make it possible to generate the MetadataDigest using "old" versions of the metadata like v14 and v15, correct?

https://github.com/bkchr/merkelized-metadata/blob/f73707f4a0d0dea8cf2c22f632eed828f244f377/src/from_frame_metadata.rs#L332-L359 this is the implementation. There are no other reasonable values for num_bytes nor least_significant_bit_first.

If that's the case, wouldn't it be beneficial to also mention in the spec what's the canonical way for accessing those field values?

I think the current assumption is that these values are just copied from the chain spec. I mean I left as an unresolved question in the RFC that this isn't really perfect. This goes into the direction of semantic information, which we are not supporting at all. It also ignores chains like asset-hub where you can pay with one of the "unlimited" number of assets. All in all not a great solution. I'm open for suggestions.

Furthermore, it would probably be a good idea to state what the canonical way is for accessing the values for the fields spec_name, spec_symbol and base58_prefix, which IIRC they are today present in the constants of the "System" pallet.

Yeah, as you said spec_name, spec_version and base58_prefix are part of the constants. They are also "real" constants per chain and don't change that much. Not sure we need to mention where to get them from? Especially as these values don't "randomly" change. A wallet could also call Core_version to get the runtime version and then fetch these values. If you think I should mention this, I can add a sentence to the RFC.

xlc commented 4 months ago

No need to put a real metadata. Just a hand crafted minimal one would do. The idea is it will be used as a test case that future implementations must ensure the outcome always matches. The same reason other spec/eip have test cases.

bkchr commented 4 months ago

There a quite a lot of different types and combinations of them that would be required to make any useful test case. I mean I'm in for ensuring that future implementation are compatible, but we should ensure this by testing these implementation against each.

bkchr commented 4 months ago

Current implementation: https://github.com/bkchr/merkleized-metadata

bkchr commented 4 months ago

/rfc propose

bkchr commented 4 months ago

/rfc propose

paritytech-rfc-bot[bot] commented 4 months ago

Hey @bkchr, here is a link you can use to create the referendum aiming to approve this RFC number 0078.

Instructions

1. Open the [link](https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fpolkadot-collectives-rpc.polkadot.io#/extrinsics/decode/0x3d003e02015901000049015246435f415050524f564528303037382c39333738616237613338373166353035316533336635346331316631613735343830653565326237373062376336633431616561366265383764373435323030290100000000). 2. Switch to the `Submission` tab.

3. Adjust the transaction if needed (for example, the proposal Origin). 4. Submit the Transaction