refactor: introduce encryption persistence interface #6998
Conversation
Jenkins BuildsClick to see older builds (34)
|
osmaczko
force-pushed
the
refactor/messaging-persistence-interfaces
branch
7 times, most recently
from
0ee8ad6 to
459e2ec
Compare
- modularize messaging Persistence with wakuPersistence, encryptionPersistence, etc. - separate migration table for encryption migrations - inject encryption sqlite persistence through the interface iterates: #6792
osmaczko
force-pushed
the
refactor/messaging-persistence-interfaces
branch
from
459e2ec to
78d6e84
Compare
|
@igor-sirotin @dlipicar it is now ready for review 🙏 |
| if _, err := db.Exec(createIndex); err != nil { | ||
| return err | ||
| } | ||
| insertVersion := fmt.Sprintf(`INSERT INTO %s (version, dirty) VALUES (?, ?)`, name) // nolint:gosec |
There was a problem hiding this comment.
How about this to avoid // nolint?
| insertVersion := fmt.Sprintf(`INSERT INTO %s (version, dirty) VALUES (?, ?)`, name) // nolint:gosec | |
| insertVersion := fmt.Sprintf(`INSERT INTO %s (version, dirty)`, name) + "VALUES (?, ?)" |
| type Persistence interface { | ||
| wakuPersistence | ||
| WakuStorage() WakuPersistence | ||
| SegmentationStorage() SegmentationPersistence | ||
| EncryptionStorage() EncryptionPersistence |
There was a problem hiding this comment.
Would it not be simpler to compose instead?
| type Persistence interface { | |
| wakuPersistence | |
| WakuStorage() WakuPersistence | |
| SegmentationStorage() SegmentationPersistence | |
| EncryptionStorage() EncryptionPersistence | |
| type Persistence interface { | |
| WakuPersistence | |
| SegmentationPersistence | |
| EncryptionPersistence |
In case of status-go it is going to be a single implementation, right?
And in case they're separate, one could still compose all implementations into one
| migrations.AssetNames(), | ||
| func(name string) ([]byte, error) { | ||
| return migrations.Asset(name) | ||
| }, |
There was a problem hiding this comment.
| migrations.AssetNames(), | |
| func(name string) ([]byte, error) { | |
| return migrations.Asset(name) | |
| }, | |
| migrations.AssetNames(), | |
| migrations.Asset, |
| ) | ||
|
|
||
| const ( | ||
| uuid = "7676fd0c" |
There was a problem hiding this comment.
Where is this magic string coming from?
There was a problem hiding this comment.
I guess it's just a random value, but please add some comment clarifying it
| if version > 0 { | ||
| err := createMigrationTables(database, version) |
There was a problem hiding this comment.
The logic with version seem to be quite convoluted here and in LatestMigrationUpToVersion 🤔
But I see GetLastMigrationVersion returns migrationTableExists here, which we ignore:
status-go/protocol/sqlite/db.go
Line 19 in 78d6e84
Maybe just check it and create the tables if they don't exist?
| func EncryptionPersistence(p types.Persistence) encryption.Persistence { | ||
| encryptionStorage := p.EncryptionStorage() | ||
| if encryptionStorage == nil { | ||
| return nil | ||
| } | ||
| internal, ok := encryptionStorage.(*internal.SQLiteEncryptionPersistence) | ||
| if !ok { | ||
| panic("custom EncryptionPersistence implementations are not supported yet") | ||
| } | ||
| return internal.SQLitePersistence | ||
| } |
There was a problem hiding this comment.
This part confuses me.
Sorry I had to write it down to consume, you probably know all of this 😄
Part 1. Interfaces
-
In
newCorewe get apersistencepointer from the user as part ofparams:
Line 79 in 78d6e84
Line 70 in 78d6e84
-
types.Persistenceis expected to provideEncryptionPersistence:
status-go/messaging/types/persistence.go
Line 10 in 78d6e84
-
Yet the
encryptionpackage expects it's own internalPersistenceinterface:
status-go/messaging/layers/encryption/persistence.go
Lines 33 to 37 in 78d6e84
-
That's why we use this
EncryptionPersistenceadapter here
Lines 92 to 93 in 78d6e84
Part 2. Adapter.
-
The adapter works, because
status-gouses the default sqlite encryption persistence:
status-go/protocol/messaging_persistence.go
Lines 253 to 255 in 78d6e84
... which, by the way is marked as "internal implementation" 🤔
status-go/messaging/sqlite_persistence.go
Lines 58 to 64 in 78d6e84
-
The implementation of
SQLiteEncryptionPersistenceis simply empty:
status-go/messaging/internal/sqlite_encryption_persistence.go
Lines 7 to 37 in 78d6e84
... yet it composes
encryption.SQLitePersistence.
And this is what the adapter relies on.
Part 3. Question
messagingtypes.EncryptionPersistence seem to be completely unused 🤔
Do we really need it? And the other persistence adapters?
Maybe we could just re-export the internal interfaces from messaging module?
3 participants
encryptionPersistence, etc.
iterates: #6792