Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finish storage format changes #1042

Merged
merged 9 commits into from
Jul 2, 2021

Conversation

turbolent
Copy link
Member

@turbolent turbolent commented Jun 30, 2021

Work towards #870

Description

  • Fix the encoding of deferred array values: Always write the CBOR value tag

  • Improve the re-encoding tests and assert the encoded and re-encoded contents are the same

  • Remove the tests for old storage formats, they are / will not be used anymore

  • Remove the old V3 decoder

  • I had incorrectly removed the delegation from the current decoder to the previous one, i.e. 567d20e and f913717.

    I reverted these changes, as I now understand what they do and why it is important:
    When we are using the previous decoder, deferred values are created – we also need to decode the deferred data with the same version of the decoder.

    @SupunS aren't we missing a check like at the top of decodeArrayMetaInfo also in decodeDictionaryMetaInfo?

    We'll need this scenario of decoding using the old decoder and re-encoding, where the arrays and dictionaries are first deferred and need to be forced to be decoded, for the state migration.
    That I was able to break this by reverting these commits and CI not failing is a sign that we're missing tests for it.

    @SupunS Could you please add tests for this ^? I've added this as a TODO to Implement StaticType() for arrays and dictionaries #870


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2021

Codecov Report

❗ No coverage uploaded for pull request base (feature/container-static-types@c3e391f). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                        Coverage Diff                        @@
##             feature/container-static-types    #1042   +/-   ##
=================================================================
  Coverage                                  ?   73.41%           
=================================================================
  Files                                     ?      271           
  Lines                                     ?    33944           
  Branches                                  ?        0           
=================================================================
  Hits                                      ?    24919           
  Misses                                    ?     7891           
  Partials                                  ?     1134           
Flag Coverage Δ
unittests 73.41% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3e391f...28d2173. Read the comment docs.

Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Strange thing with the missing delegations for decodeDictionaryMetaInfo. It was added in f00489b and seems to be still there in my local branch. Probably Git overwrote it with another commit somehow during merging?

I will add it back, and some tests too. Thanks for catching that!

@@ -799,6 +799,15 @@ func (e *EncoderV5) encodeArray(
deferrals *EncodingDeferrals,
) error {

// Encode tag number and array head
err := e.enc.EncodeRawBytes([]byte{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

@SupunS
Copy link
Member

SupunS commented Jul 2, 2021

Added the missing check and some tests: 28d2173.

@SupunS SupunS merged commit f8f09b6 into feature/container-static-types Jul 2, 2021
@SupunS SupunS deleted the bastian/finish-storage-v5 branch July 2, 2021 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants