Signing and SIDs via Registrant, refactor tests #112
Conversation
| db, err := db.NewDB( | ||
| ctx, | ||
| options.DB.WriterConnectionString, | ||
| options.DB.WaitForDB, | ||
| options.DB.ReadTimeout, | ||
| ) | ||
| if err != nil { | ||
| log.Fatal("initializing database", zap.Error(err)) | ||
| } | ||
|
|
||
| s, err := server.NewReplicationServer( | ||
| ctx, | ||
| log, | ||
| options, | ||
| registry.NewFixedNodeRegistry([]registry.Node{}), | ||
| db, | ||
| ) |
There was a problem hiding this comment.
Pulled the DB out of the replication server for dependency injection purposes. Otherwise, multiple instantiated servers in a test would share the same database and/or have cleanup issues.
| -- name: InsertNodeInfo :one | ||
| INSERT INTO node_info(node_id, public_key) | ||
| VALUES (@node_id, @public_key) | ||
| RETURNING *; | ||
|
|
||
| -- name: SelectNodeInfo :one | ||
| SELECT * FROM node_info WHERE singleton_id = 1; |
There was a problem hiding this comment.
These could have been combined into a single query that fetches the existing record when there is a conflict. Unfortunately, it looks like the methods for doing this in Postgres either impact readability significantly or have weird caveats.
I chose to do this the simple way because server startup performance isn't critical here.
There was a problem hiding this comment.
Why does this need to live in the database at all? Can't the NodeRegistry just serve as the source of truth here? Is it purely so that we can error if you change your NodeID/Public Key with the same DB?
There was a problem hiding this comment.
Yes, purely so that you get an error if you change either piece of information with the same DB. I imagine it's an easy mistake for partners to make
There was a problem hiding this comment.
OK. Good to be defensive here. Could definitely see it being an easy mistake to make.
There was a problem hiding this comment.
I just realized, the whole reason I had to refactor the tests was because they weren't passing due to multiple server nodes with different private keys sharing the same DB. So, also an easy mistake for us to make!
| db *queries.Queries, | ||
| nodeRegistry registry.NodeRegistry, | ||
| privateKeyString string, | ||
| ) (*Registrant, error) { |
There was a problem hiding this comment.
Will split this into util methods
| server1 := NewTestServer(t, dbs[0], registry, privateKey1) | ||
| server2 := NewTestServer(t, dbs[1], registry, privateKey2) |
There was a problem hiding this comment.
Here we have multiple nodes running with separate DB/private keys but a shared registry
| @@ -1,4 +1,4 @@ | |||
| package registry | |||
| package registry_test | |||
There was a problem hiding this comment.
Needed to remove circular dependencies - otherwise this test (in the registry package) depends on the mocks package, which depends on the registry package
pkg/utils/sid.go
Outdated
| const ( | ||
| nodeIDMask uint64 = 0xFFFF << 48 | ||
| nodeIDBits uint64 = 16 |
There was a problem hiding this comment.
It would be good to have some really clear comments around how these IDs are supposed to work, since bitmasks are not the most readable.
Apologies, this one exploded in size once I realized I had to refactor the tests. This PR: