-
Notifications
You must be signed in to change notification settings - Fork 132
[group key addrs 4/5]: implement mailbox server and message store database #1614
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
Conversation
094b31e
to
3d1a126
Compare
if err != nil { | ||
return nil, fmt.Errorf("error storing message: %w", err) | ||
} | ||
|
||
// Publish the message to all subscribers, meaning it will be | ||
// distributed to all subscribed streams. Each stream will filter by | ||
// recipient ID by itself. | ||
msg.ID = msgID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We assign msg.ID
to the db table’s auto-incrementing primary key. Exposing that surrogate key externally couples the database to the API.
Another possible solution: generate a UUIDv4 for each message instead, store it in the db row, and keep the integer primary key internal to the db only. This decouples external identifiers from the DB’s bookkeeping (makes table rebuilds/merge safer because message IDs never collide?).
Not a big deal, but something that might be worth exploring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually even use this ID on the proto layer?
IIUC, the client uses a block height range to scan for new messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use the ID to query as well. That makes things a bit easier if you know exactly which ID you received last. Because there could be multiple messages per block, so just using the block height might give you messages you've already seen.
Having a comparable (e.g. numeric) ID is useful here, exactly for the above case. Why do you think exposing the database primary ID is a bad thing on the API level, @ffranr? It's really just a number...
@@ -234,13 +236,14 @@ func (s *Server) SendMessage(ctx context.Context, | |||
// We didn't have the proof before, so we store it now. If at | |||
// the same time a different goroutine is trying to store the | |||
// same proof, we expect the database to handle the concurrency. | |||
err = s.cfg.TxProofStore.StoreProof(txProof.ClaimedOutPoint) | |||
err = s.cfg.TxProofStore.StoreProof(ctx, *txProof) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Needs to be rebased due to recent merges increasing the highest migration number. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐩
-- The timestamp when the message was created on the server. This is a unix | ||
-- timestamp in seconds to allow for easy querying and sorting of messages | ||
-- based on their arrival time, without time zone complications. | ||
arrival_timestamp BIGINT NOT NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
// Add context to unique constraint errors. | ||
var uniqueConstraintErr *ErrSqlUniqueConstraintViolation | ||
if errors.As(dbErr, &uniqueConstraintErr) { | ||
return 0, proof.ErrTxMerkleProofExists |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
ReadOnly() bool | ||
} | ||
|
||
// BaseTxOptions defines the set of db txn options the database understands. | ||
type BaseTxOptions struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. TBH, we can prob get rid of all the specific tx options we have like BaseUniverseStoreOptions
and just use this instead. Non-blocking can do it in some other PR as it'll increase the diff size and scope here.
@@ -418,9 +419,9 @@ func UnmarshalTxProof( | |||
type TxProofStore interface { | |||
// HaveProof returns true if the proof for the given outpoint exists in | |||
// the store. | |||
HaveProof(wire.OutPoint) (bool, error) | |||
HaveProof(context.Context, wire.OutPoint) (bool, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
636d80d
to
dc426c6
Compare
Pull Request Test Coverage Report for Build 15873763187Details
💛 - Coveralls |
This commit implements the MailboxStore which implements a SQL based implementation for the authmailbox.MsgStore and proof.TxProofStore interfaces.
To avoid collisions with the universerpc.Info RPC method, we rename our authmailbox Info RPC method to MailboxInfo.
We now plug everything together and make sure that the auth mailbox server works by adding a new integration test.
dc426c6
to
74953ff
Compare
The rapid generator for strings seems to occasionally produce invalid UTF-8 strings, causing an error with the Postgres driver. We fix that by using hex encoded bytes instead.
Depends on #1611.
Extracted some commits from #1587.
Implements the SQL store for the auth mailbox server and fully hooks up the server in
tapd
.The implementation is covered by a basic message send and receive integration test.