feat(pkg/scale): encoding and decoding of maps in scale

[axaysagathiya]

Changes

written the methods for encoding and decoding maps in scale.

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

#2796

Primary Reviewer

@timwu20

[kishansagathiya]

Fix this test Test_unmarshal_optionality/testMap1

[qdm12]

Nice, thanks 👍
Note one test is failing in the CI.

[qdm12]

Shouldn't we return an error in that case, to avoid silently discarding a map value when encoding?

@timwu20

[jimjbrettj]

Def wait for Tims answer, but this happens at other places in this file as well (i.e. structs)

[timwu20]

CanInterface is essentially determining if this is a public attribute of a struct or public method. I think it should be fine in this case. This will almost always return true in the case we're trying to decode into a map value. I wonder what cases this would return false though, maybe we can provide a test case.

@kishansagathiya please don't merge the PR with unresolved conversations. My bad for not getting to this earlier.

[kishansagathiya]

I realised about this comment few moments after merging it. Wasn't deliberate.

[EclesioMeloJunior]

Nice PR! I would like to suggest providing more test cases, for example a map from string to a struct, like:

type exampleStruct struct {
  Acc string
  Value int
}

toEncode := map[string]exampleStruct{}

[codecov]

Codecov Report

Merging #2894 (d69371d) into development (08c4281) will decrease coverage by 0.10%.
The diff coverage is 61.53%.

Additional details and impacted files

@@               Coverage Diff               @@
##           development    #2894      +/-   ##
===============================================
- Coverage        63.40%   63.30%   -0.11%     
===============================================
  Files              219      219              
  Lines            27391    27443      +52     
===============================================
+ Hits             17368    17373       +5     
- Misses            8431     8471      +40     
- Partials          1592     1599       +7     

[jimjbrettj]

Seems rpc_03-chain_test is failing

[jimjbrettj]

If this code is all in the same place, maybe better to provide a link? Not sure, just a thought

[axaysagathiya]

This is Rust code used to get expected encoded data.

[EclesioMeloJunior]

@axaysagathiya I think it is not needed to put this comment block here, could you provide a link to the actual Rust code that does the encode?

[axaysagathiya]

@EclesioMeloJunior this Rust code is written by me to get scale encoded value. Do you want me to provide a Gist link ?

[EclesioMeloJunior]

Not really, I mean your comment starts with:

// // Rust code to encode a map of string to struct.

But actually, your comment does not explain about the encoding, you're just adding two entries into a BTreeMap, so I would suggest removing this comment

[kishansagathiya]

@jimjbrettj @EclesioMeloJunior
There is no link. This was the actual code.

you're just adding two entries into a BTreeMap

No, It is also encoding that map and printing it.

Look at this line ec37a64#diff-c0f41b36f6b47167cef1d9b4a3453b021f9e2337e9e7a3cdbc92cae8e4c62ffdL161.
Execute that code and you would be able to see it in action.

Unlike go, rust does not have a scale.Encode method. You have to add Encode into derive attribute of the struct and then you can directly do name_of_the_obj.encode()
Sort of like this

#[derive(Encode, Decode)]
struct Foo<T> {
    a: i32,
    b: T,
}

I would like this code context to be there. Someday when we have to edit this test, it would be easier to generate encoding values, otherwise it will be a black box how those values came to be.

[jimjbrettj]

@kishansagathiya cool, I thought it was taken from somewhere else so I am fine to leave it as is. Want to avoid situations where we don't know where bytes came from.

[EclesioMeloJunior]

Oh, okay I did understand that you were trying to explain how rust encodes but you're explaining how you generated the encoded bytes that you're using in the test (my bad 😅)

[kishansagathiya]

Oh, okay I did understand that you were trying to explain how rust encodes but you're explaining how you generated the encoded bytes that you're using in the test (my bad sweat_smile)

no problem

@axaysagathiya you can bring the commented code back, sorry about the confusion.

[jimjbrettj]

Great work, just have a few questions

[EclesioMeloJunior]

@axaysagathiya I think it is not needed to put this comment block here, could you provide a link to the actual Rust code that does the encode?

[timwu20]

why are we doing this? Is this just so we can have deterministic tests?

[axaysagathiya]

yeah

[timwu20]

@axaysagathiya can you create another PR to only do this in the tests? This is not required when encoding maps in production, and it's not very performant. I'd suggest adding a sortKeys parameter to the encodeMap function signature.

[axaysagathiya]

Oh, my bad!

This is not required when encoding maps

This is not just needed in test, this is needed in encoding as well. keys in go maps are unordered. so running encode function on the same go map could result into different encodings. Rust equivalent of go map is BTree map, where keys are sorted. Scale encode function in the Rust implementation does not produces different values on running it multiple times.

So, in order to make sure that our scale package and Rust's scale package encodes a map to the same value sorting is necessary.

[timwu20]

Just because the BTreeMap has deterministic sorting of keys, doesn't mean that the parity-scale-codec isn't able to decode maps that were encoded with keys in non-deterministic order. In go it's understood that the order of the keys is non-deterministic. In this case, we should be testing that we can encode a map and decode into a map, and assert that the maps are equal. We could also ensure that decoding into a rust BTreeMap via the parity-scale-codec works as expected. To make the tests more deterministic, you could provide a map with a single key or a map with two keys, and just assert that it's equal to one of the two encodings and remove the sorting logic altogether.

[github-actions]

🎉 This PR is included in version 0.7.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀