refactor: modularize messaging persistence #6998
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Jenkins BuildsClick to see older builds (85)
|
osmaczko
force-pushed
the
refactor/messaging-persistence-interfaces
branch
7 times, most recently
from
0ee8ad6 to
459e2ec
Compare
osmaczko
force-pushed
the
refactor/messaging-persistence-interfaces
branch
from
459e2ec to
78d6e84
Compare
Contributor
Author
|
@igor-sirotin @dlipicar it is now ready for review 🙏 |
Collaborator
igor-sirotin
left a comment
igor-sirotin
left a comment
There was a problem hiding this comment.
Didn't look the whole code yet, this is the first part of comments
sqlite/migrate.go
Outdated
| if _, err := db.Exec(createIndex); err != nil { | ||
| return err | ||
| } | ||
| insertVersion := fmt.Sprintf(`INSERT INTO %s (version, dirty) VALUES (?, ?)`, name) // nolint:gosec |
Collaborator
There was a problem hiding this comment.
How about this to avoid // nolint?
Suggested change
| insertVersion := fmt.Sprintf(`INSERT INTO %s (version, dirty) VALUES (?, ?)`, name) // nolint:gosec | |
| insertVersion := fmt.Sprintf(`INSERT INTO %s (version, dirty)`, name) + "VALUES (?, ?)" |
osmaczko
force-pushed
the
refactor/messaging-persistence-interfaces
branch
from
78d6e84 to
557faaa
Compare
osmaczko
changed the title
osmaczko
force-pushed
the
refactor/messaging-persistence-interfaces
branch
9 times, most recently
from
99abe84 to
6445824
Compare
igor-sirotin
approved these changes
Oct 22, 2025
Collaborator
igor-sirotin
left a comment
igor-sirotin
left a comment
There was a problem hiding this comment.
Nothing critical, just 19 little comments 😂
Great job! 👏
Would be amazing if you can write down the approach in some README in the repo. So that further we can follow it with other modules.
protocol/migrations/sqlite/1712905223_add_parity_to_message_segments.up.sql
Outdated
Show resolved
Hide resolved
Comment on lines
+30
to
+34
| db, err := helpers.SetupTestMemorySQLDB(helpers.NewTestDBInitializer([]*bindata.AssetSource{ | ||
| { | ||
| Names: migrations.AssetNames(), | ||
| AssetFunc: migrations.Asset, | ||
| }, |
Collaborator
There was a problem hiding this comment.
Maybe we can implement
package migrations
func AssetSources() []*bindata.AssetSource {
return []*bindata.AssetSource{
{
Names: AssetNames(),
AssetFunc: Asset,
},
}
}
Contributor
There was a problem hiding this comment.
Agree, we see these two lines repeated in a lot of places, would be nice to make it a single one
Contributor
Author
There was a problem hiding this comment.
migrations is a generated package. I’d have to create a separate file for each module containing migrations and define an identical AssetSources() function, which is also far from ideal. For now, I prefer duplication for the sake of simplicity, wdyt?
messaging/waku/migrations/sqlite/1691753800_pubsubtopic_key.up.sql
Outdated
Show resolved
Hide resolved
dlipicar
reviewed
Oct 22, 2025
dlipicar
approved these changes
Oct 22, 2025
- make each messaging submodule define its own persistence interface and SQLite implementation reference - separate migration tables for messaging submodules - move migrations from protocol to relevant messaging submodules closes: #6792
osmaczko
force-pushed
the
refactor/messaging-persistence-interfaces
branch
from
6445824 to
18059a8
Compare
osmaczko
added a commit
to status-im/status-desktop
that referenced
this pull request
Oct 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SQLite implementation reference
Explanation of how to move existing migrations to submodules: https://miro.com/app/board/uXjVJ3l1qdo=/?moveToWidget=3458764645112517555&cot=14
closes: #6792