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.

Sign up for GitHub

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

Jump to bottom

Add support for Maps to the reflect_codec #1790

Merged
merged 32 commits into from
Aug 11, 2023
Merged

Add support for Maps to the reflect_codec #1790

merged 32 commits into from
Aug 11, 2023

Conversation

nytzuga
Copy link
Contributor

@nytzuga nytzuga commented Aug 2, 2023 •
edited
Loading

Support to serialize/deserialize maps with the reflect_codec. This is needed in the subnet EMV to use it to exchange information about the upgrade configs.

This PR adds support for maps and tests it.

The internal binary format is quite simple

    [length: 4 bytes][key1][value1][key2][value2]

The length is the total number of elements and keys, therefore it should always be a pair number.

Why this should be merged

How this works

How this was tested

@nytzuga nytzuga requested a review from danlaine as a code owner August 2, 2023 00:07
@nytzuga nytzuga force-pushed the add-support-for-map-into-reflectcodec branch 3 times, most recently from 726dc65 to e526eba Compare August 2, 2023 00:53
@patrick-ogrady patrick-ogrady marked this pull request as draft August 2, 2023 04:22
@nytzuga nytzuga changed the title Draft: Add support for Maps to the reflect_codec Add support for Maps to the reflect_codec Aug 2, 2023
@nytzuga nytzuga self-assigned this Aug 2, 2023
ceyonur
ceyonur requested changes Aug 2, 2023
View reviewed changes
Copy link
Contributor

@ceyonur ceyonur left a comment

Choose a reason for hiding this comment

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

A great feature for codec 💯

codec/reflectcodec/type_codec.go Show resolved Hide resolved
codec/reflectcodec/type_codec.go Show resolved Hide resolved
codec/reflectcodec/type_codec.go Outdated Show resolved Hide resolved
codec/reflectcodec/type_codec.go Show resolved Hide resolved
codec/reflectcodec/type_codec.go Outdated Show resolved Hide resolved
codec/test_codec.go Outdated Show resolved Hide resolved
danlaine
danlaine suggested changes Aug 2, 2023
View reviewed changes
Copy link

@danlaine danlaine left a comment

Choose a reason for hiding this comment

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

see comments

@nytzuga nytzuga force-pushed the add-support-for-map-into-reflectcodec branch from a7c556e to acf8c70 Compare August 2, 2023 21:11
@nytzuga nytzuga force-pushed the add-support-for-map-into-reflectcodec branch 3 times, most recently from 734c5ce to c940c9d Compare August 3, 2023 03:46
ceyonur
ceyonur reviewed Aug 3, 2023
View reviewed changes
codec/reflectcodec/type_codec.go Outdated Show resolved Hide resolved
codec/reflectcodec/type_codec.go Outdated Show resolved Hide resolved
codec/reflectcodec/type_codec.go Outdated Show resolved Hide resolved
codec/reflectcodec/type_codec.go Outdated Show resolved Hide resolved
codec/test_codec.go Show resolved Hide resolved
@nytzuga nytzuga requested a review from ceyonur August 3, 2023 16:46
@nytzuga nytzuga marked this pull request as ready for review August 3, 2023 17:00
danlaine
danlaine reviewed Aug 3, 2023
View reviewed changes
codec/reflectcodec/type_codec.go Outdated Show resolved Hide resolved
codec/reflectcodec/type_codec.go Outdated Show resolved Hide resolved
@nytzuga nytzuga force-pushed the add-support-for-map-into-reflectcodec branch from eb8fc0f to 287b05f Compare August 11, 2023 15:15
@StephenButtolph StephenButtolph changed the base branch from master to dev August 11, 2023 19:00
@StephenButtolph StephenButtolph added this to the v1.10.9 milestone Aug 11, 2023
@StephenButtolph StephenButtolph added the sdk This involves SDK tooling or frameworks label Aug 11, 2023
dhrubabasu
dhrubabasu reviewed Aug 11, 2023
View reviewed changes
codec/test_codec.go Outdated Show resolved Hide resolved
@nytzuga @dhrubabasu
Update codec/test_codec.go
374bbe7 
Co-authored-by: Dhruba Basu <7675102+dhrubabasu@users.noreply.github.com>
Signed-off-by: Cesar <137245636+nytzuga@users.noreply.github.com>
@StephenButtolph StephenButtolph merged commit dfa3009 into dev Aug 11, 2023
14 checks passed
@StephenButtolph StephenButtolph deleted the add-support-for-map-into-reflectcodec branch August 11, 2023 22:29
nytzuga added a commit to ava-labs/subnet-evm that referenced this pull request Aug 14, 2023
@nytzuga
Add handshake codec
50741b9 
The goal of the this codec is to serialize `params.UpgradeConfig` in a
deterministic way, to be hashed later. This hash is going to be share by nodes
at handshake, so they can determine if they have the same upgrade config.

This PR leverages the newly introduced support to serilize Maps
ava-labs/avalanchego#1790, which are deterministic (the
values are sorted by keys before serializing)
@nytzuga nytzuga mentioned this pull request Aug 14, 2023
Draft
nytzuga added a commit to ava-labs/subnet-evm that referenced this pull request Aug 14, 2023
@nytzuga
Add handshake codec
de94050 
The goal of the this codec is to serialize `params.UpgradeConfig` in a
deterministic way, to be hashed later. This hash is going to be share by nodes
at handshake, so they can determine if they have the same upgrade config.

This PR leverages the newly introduced support to serilize Maps
ava-labs/avalanchego#1790, which are deterministic (the
values are sorted by keys before serializing)
nytzuga added a commit to ava-labs/subnet-evm that referenced this pull request Aug 14, 2023
@nytzuga
Add handshake codec
a9dcdd7 
The goal of the this codec is to serialize `params.UpgradeConfig` in a
deterministic way, to be hashed later. This hash is going to be share by nodes
at handshake, so they can determine if they have the same upgrade config.

This PR leverages the newly introduced support to serilize Maps
ava-labs/avalanchego#1790, which are deterministic (the
values are sorted by keys before serializing)
nytzuga added a commit to ava-labs/subnet-evm that referenced this pull request Aug 14, 2023
@nytzuga
DO NOT MERGE: Temporary commit
6740f23 
Update the reference to avalanchego@dev to test the feature. The needed feature
ava-labs/avalanchego#1790 is only supported in dev.
nytzuga added a commit to ava-labs/subnet-evm that referenced this pull request Sep 8, 2023
@nytzuga
Add handshake codec
be9f6b2 
The goal of the this codec is to serialize `params.UpgradeConfig` in a
deterministic way, to be hashed later. This hash is going to be share by nodes
at handshake, so they can determine if they have the same upgrade config.

This PR leverages the newly introduced support to serilize Maps
ava-labs/avalanchego#1790, which are deterministic (the
values are sorted by keys before serializing)
nytzuga added a commit to ava-labs/subnet-evm that referenced this pull request Sep 14, 2023
@nytzuga
Add handshake codec
f656e22 
The goal of the this codec is to serialize `params.UpgradeConfig` in a
deterministic way, to be hashed later. This hash is going to be share by nodes
at handshake, so they can determine if they have the same upgrade config.

This PR leverages the newly introduced support to serilize Maps
ava-labs/avalanchego#1790, which are deterministic (the
values are sorted by keys before serializing)
nytzuga added a commit to ava-labs/subnet-evm that referenced this pull request Sep 26, 2023
@nytzuga
Add handshake codec
961c366 
The goal of the this codec is to serialize `params.UpgradeConfig` in a
deterministic way, to be hashed later. This hash is going to be share by nodes
at handshake, so they can determine if they have the same upgrade config.

This PR leverages the newly introduced support to serilize Maps
ava-labs/avalanchego#1790, which are deterministic (the
values are sorted by keys before serializing)
nytzuga added a commit to ava-labs/subnet-evm that referenced this pull request Oct 11, 2023
@nytzuga
Add handshake codec
86cb190 
The goal of the this codec is to serialize `params.UpgradeConfig` in a
deterministic way, to be hashed later. This hash is going to be share by nodes
at handshake, so they can determine if they have the same upgrade config.

This PR leverages the newly introduced support to serilize Maps
ava-labs/avalanchego#1790, which are deterministic (the
values are sorted by keys before serializing)
nytzuga added a commit to ava-labs/subnet-evm that referenced this pull request Oct 11, 2023
@nytzuga
Add handshake codec
a095515 
The goal of the this codec is to serialize `params.UpgradeConfig` in a
deterministic way, to be hashed later. This hash is going to be share by nodes
at handshake, so they can determine if they have the same upgrade config.

This PR leverages the newly introduced support to serilize Maps
ava-labs/avalanchego#1790, which are deterministic (the
values are sorted by keys before serializing)
nytzuga added a commit to ava-labs/subnet-evm that referenced this pull request Oct 13, 2023
@nytzuga
Add handshake codec
ab26147 
The goal of the this codec is to serialize `params.UpgradeConfig` in a
deterministic way, to be hashed later. This hash is going to be share by nodes
at handshake, so they can determine if they have the same upgrade config.

This PR leverages the newly introduced support to serilize Maps
ava-labs/avalanchego#1790, which are deterministic (the
values are sorted by keys before serializing)
nytzuga added a commit to ava-labs/subnet-evm that referenced this pull request Nov 2, 2023
@nytzuga
Add handshake codec
0f72644 
The goal of the this codec is to serialize `params.UpgradeConfig` in a
deterministic way, to be hashed later. This hash is going to be share by nodes
at handshake, so they can determine if they have the same upgrade config.

This PR leverages the newly introduced support to serilize Maps
ava-labs/avalanchego#1790, which are deterministic (the
values are sorted by keys before serializing)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danlaine danlaine danlaine approved these changes

@ceyonur ceyonur ceyonur approved these changes

@dhrubabasu dhrubabasu dhrubabasu approved these changes

@StephenButtolph StephenButtolph StephenButtolph approved these changes

@aaronbuchwald aaronbuchwald Awaiting requested review from aaronbuchwald

@darioush darioush Awaiting requested review from darioush

Assignees

@nytzuga nytzuga

Labels
sdk This involves SDK tooling or frameworks
Projects
No open projects
Ski Patrol
Archived in project
Milestone
v1.10.9
Development

Successfully merging this pull request may close these issues.
7 participants
@nytzuga @darioush @dhrubabasu @danlaine @ceyonur @StephenButtolph @aaronbuchwald