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

Merge feature/protocol-state-kvstore to master #5840

Merged
merged 239 commits into from
May 6, 2024

Conversation

jordanschalm
Copy link
Member

@jordanschalm jordanschalm commented May 3, 2024

Merges the Protocol State KV Store feature branch to master.

There were many conflicts reported by Git. The vast majority were changes on master that had no conflicting change on the feature branch. There were some conflicts in integration test suite (some changed method names and re-organization) and in tools and API methods for retrieving a protocol snapshot.

The full list of conflicting files is in the description of the merge commit: 81b938e.

Changes made after the merge commit are in this diff

Outstanding TODOs

durkmurder and others added 30 commits February 9, 2024 17:20
Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>
* begin adding kvstore types

* wip

* sketching out versioned encoding

* re-organize models and interfaces

* update docs, tests

* correct pointer in encoding test

* lint: goimports

* Update state/protocol/protocol_state/kvstore/interfaces.go

Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>

---------

Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>
Change to encode/decode passes tests, still requires cleanup:
- adds encode/decode for new event type
- adds tests for new event type
- changes how decoding is implemented to prevent a bug. Previously we
  would re-encode, then re-decode the entire service event, after first
  decoding it into a weakly typed map. In particular with JSON, this
  could cause the data to change between encode/decode cycles, I think because
  of numeric type sizes.
Copy link
Contributor

@peterargue peterargue left a comment

Choose a reason for hiding this comment

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

Added one comment about an unneeded access change. otherwise this looks good from the AN perspective.

cmd/bootstrap/cmd/finalize_test.go Outdated Show resolved Hide resolved
engine/common/rpc/convert/transactions.go Outdated Show resolved Hide resolved
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

Did a cursory review of changes interacting with the Cadence 1.0 and migration code, and left one question regarding the core contract dependencies.

go.mod Outdated Show resolved Hide resolved
cmd/bootstrap/cmd/finalize_test.go Outdated Show resolved Hide resolved
cmd/util/ledger/migrations/utils.go Outdated Show resolved Hide resolved
fvm/evm/testutils/contracts/test_bytes.hex Outdated Show resolved Hide resolved
integration/benchmark/server/flow-go Outdated Show resolved Hide resolved
model/convert/fixtures_test.go Outdated Show resolved Hide resolved
Copy link
Member

@AlexHentschel AlexHentschel left a comment

Choose a reason for hiding this comment

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

thanks for taking on the merge. Didn't find anything.

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice! 👏

@jordanschalm jordanschalm added this pull request to the merge queue May 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 6, 2024
@jordanschalm jordanschalm added this pull request to the merge queue May 6, 2024
Merged via the queue into master with commit 1388e44 May 6, 2024
55 checks passed
@jordanschalm jordanschalm deleted the jord/sync-kvstore-master branch May 6, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants