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

Ler2block #61

Merged
merged 33 commits into from
Sep 13, 2024
Merged

Ler2block #61

merged 33 commits into from
Sep 13, 2024

Conversation

arnaubennassar
Copy link
Contributor

@arnaubennassar arnaubennassar commented Sep 4, 2024

Description

  • mdbx to SQLite for [tree, l1infotreesync, bridgesync]
  • migrations support
  • meddler for clean and easy SQL <==> struct
  • moved types from tree to tree/types to avoid cycles + created type proof (cleaner + easier for SQL)

NOTE: change base branch to develop once feature/sync-imported-bridges is merged

Copy link
Contributor

@joanestebanr joanestebanr left a comment

Choose a reason for hiding this comment

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

I just review /tree, but I think that the comments can be extended to rest

claimsponsor/e2e_test.go Outdated Show resolved Hide resolved
tree/tree.go Outdated Show resolved Hide resolved
tree/tree.go Show resolved Hide resolved
tree/tree.go Outdated Show resolved Hide resolved
tree/appendonlytree.go Show resolved Hide resolved
tree/migrations/migrations.go Outdated Show resolved Hide resolved
Base automatically changed from feature/sync-imported-bridges to develop September 5, 2024 09:11
bridgesync/processor.go Outdated Show resolved Hide resolved
bridgesync/processor.go Outdated Show resolved Hide resolved
require.Equal(t, a.expectedErr, actualErr)
}

// GetBridges
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the comment.

db/meddler.go Outdated Show resolved Hide resolved
db/meddler.go Outdated Show resolved Hide resolved
db/meddler.go Outdated Show resolved Hide resolved
db/meddler.go Outdated Show resolved Hide resolved
db/meddler.go Outdated Show resolved Hide resolved
l1infotreesync/migrations/migrations.go Outdated Show resolved Hide resolved
tree/appendonlytree.go Show resolved Hide resolved
@arnaubennassar arnaubennassar self-assigned this Sep 10, 2024
@@ -0,0 +1,35 @@
package migrations
Copy link
Contributor

Choose a reason for hiding this comment

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

This file and ./l1infotreesync/migrations/migrations.go looks pretty much the same.. is not possible to move this to db package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO it's syncer has it's own migrations, it would be odd IMO to have a "centralized" package that imports the migrations files of each syncer, and has functions to run those specific for each syncer

@@ -26,7 +27,7 @@ func NewAppendOnlyTree(db *sql.DB, dbPrefix string) *AppendOnlyTree {
}
}

func (t *AppendOnlyTree) AddLeaf(tx *sql.Tx, blockNum, blockPosition uint64, leaf types.Leaf) error {
func (t *AppendOnlyTree) AddLeaf(tx *db.Tx, blockNum, blockPosition uint64, leaf types.Leaf) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to use a interface instead real object, It allows to unittest more easy and have multiples implementation (as the case of zkevm-sync-l1 that have one implementation of posgtres and another for sqlite)

@@ -72,10 +73,11 @@ func (t *AppendOnlyTree) AddLeaf(tx *sql.Tx, blockNum, blockPosition uint64, lea
return err
}
t.lastIndex++
tx.AddRollbackCallback(func() { t.lastIndex-- })
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you can't execute in parallel this code, isn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, there is a single writer, and it's sequential (it flows form ProcessBlock of each syncer)

@@ -184,7 +184,7 @@ func (p *processor) getLastProcessedBlockWithTx(tx db.DBer) (uint64, error) {
// Reorg triggers a purge and reset process on the processor to leaf it on a state
// as if the last block processed was firstReorgedBlock-1
func (p *processor) Reorg(ctx context.Context, firstReorgedBlock uint64) error {
tx, err := p.db.BeginTx(ctx, nil)
tx, err := db.NewTx(ctx, p.db)
Copy link
Contributor

Choose a reason for hiding this comment

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

I sugest that newTx belong to DB object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but db is *sql.DB, I can't add this method there 😅

Copy link

sonarcloud bot commented Sep 13, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
77.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@arnaubennassar arnaubennassar merged commit 8a74627 into develop Sep 13, 2024
6 of 9 checks passed
@arnaubennassar arnaubennassar deleted the ler2block branch September 13, 2024 13:58
Stefan-Ethernal pushed a commit that referenced this pull request Sep 17, 2024
* wip

* wip

* WIP

* decoding direct and indeirecr assets and messages works

* connect everything

* fix building contract scripts

* fix building contract scripts

* wip

* WIP

* tree migrated to SQLite

* wip

* wip

* bridgesync working with sqlite

* pass tests

* minor cleaning

* add GetBlockByLER func

* handle err not found

* merge develop

* use memory for sqlite on the tests

* increase timestamp to pass UT

* review

* add callbacks on db tx

* lint

* fix compilation

* fix linter II

* fix linter III

* fix linter

* increase linter TO

* fix UTs and lint

* increase linter TO

* add PR requests
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.

4 participants