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

feat: implement SQL lite storage #72

Conversation

Stefan-Ethernal
Copy link
Collaborator

@Stefan-Ethernal Stefan-Ethernal commented Sep 27, 2024

What does this PR do?

This PR introduces SQL lite storage to the zkevm-eth-txmanager, in case the PersistenceFilename config param is provided.

Reviewers

Main reviewers:

common/common.go Outdated Show resolved Hide resolved
ethtxmanager/sqlstorage/sqlstorage.go Outdated Show resolved Hide resolved
Copy link

@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'm wondering if make sense to support dbTx?

ethtxmanager/sqlstorage/sqlstorage.go Show resolved Hide resolved
ethtxmanager/sqlstorage/sqlstorage.go Show resolved Hide resolved
ethtxmanager/sqlstorage/sqlstorage.go Outdated Show resolved Hide resolved
ethtxmanager/sqlstorage/sqlstorage.go Outdated Show resolved Hide resolved
ethtxmanager/sqlstorage/sqlstorage.go Outdated Show resolved Hide resolved
ethtxmanager/memory_storage/memstorage.go Outdated Show resolved Hide resolved
ethtxmanager/sqlstorage/migrations/0001.sql Outdated Show resolved Hide resolved
ethtxmanager/sqlstorage/migrations/0001.sql Outdated Show resolved Hide resolved
ethtxmanager/sqlstorage/migrations/0001.sql Show resolved Hide resolved
ethtxmanager/sqlstorage/sqlstorage.go Outdated Show resolved Hide resolved
@Stefan-Ethernal
Copy link
Collaborator Author

Stefan-Ethernal commented Oct 1, 2024

I'm wondering if make sense to support dbTx?

Can you elaborate some more? Do you mean to be able to do multiple changes in the same transaction (basically to ensure transactional mode)?

IMO, probably doesn't make too much sense, because it is only a single table, but I'm not sure (it depends of the usage). There might be some valid use case for it 🤔

@joanestebanr
Copy link

I'm wondering if make sense to support dbTx?

Can you elaborate some more? Do you mean to be able to do multiple changes in the same transaction (basically to ensure transactional mode)?

IMO, probably doesn't make too much sense, because it is only a single table, but I'm not sure (it depends of the usage). There might be some valid use case for it 🤔

I don't known that is done internally but even with just 1 table make sense:

  • You do multiples INSERTS but want to rollback if any of them fails
  • You do some INSERTS and check something over them in another function (that need to be in the same set of data)
    Basically, it depends how works internally. But in the future make sense, that if we modify it, we think in that model because is the common in the rest of Database usage

@Stefan-Ethernal
Copy link
Collaborator Author

I don't known that is done internally but even with just 1 table make sense:

You do multiples INSERTS but want to rollback if any of them fails
You do some INSERTS and check something over them in another function (that need to be in the same set of data)
Basically, it depends how works internally. But in the future make sense, that if we modify it, we think in that model because is the common in the rest of Database usage

I discussed this with @goran-ethernal and @joanestebanr, there is no use case, where we would benefit from introducing transactional model. The transactions are affected one by one.

@Stefan-Ethernal Stefan-Ethernal force-pushed the CDK-501-seq-sender-change-current-json-file-ethtxmanager-json-for-a-database-sqlite branch from 26fc997 to d3db367 Compare October 2, 2024 12:15
ethtxmanager/sqlstorage/sqlstorage.go Outdated Show resolved Hide resolved
ethtxmanager/sqlstorage/sqlstorage.go Show resolved Hide resolved
@goran-ethernal goran-ethernal force-pushed the CDK-501-seq-sender-change-current-json-file-ethtxmanager-json-for-a-database-sqlite branch from 4c36837 to db40fc9 Compare October 2, 2024 14:27
ethtxmanager/sqlstorage/sqlstorage.go Outdated Show resolved Hide resolved
@Stefan-Ethernal Stefan-Ethernal force-pushed the CDK-501-seq-sender-change-current-json-file-ethtxmanager-json-for-a-database-sqlite branch from d136818 to c009e71 Compare October 2, 2024 19:54
Copy link

sonarcloud bot commented Oct 2, 2024

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarCloud

@Stefan-Ethernal Stefan-Ethernal merged commit ec887ce into main Oct 3, 2024
5 of 6 checks passed
@Stefan-Ethernal Stefan-Ethernal deleted the CDK-501-seq-sender-change-current-json-file-ethtxmanager-json-for-a-database-sqlite branch October 3, 2024 06:00
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.

5 participants