Link payer to gateway envelopes #536
Conversation
|
Warning Rate limit exceeded@neekolas has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (12)
WalkthroughThe changes introduce a new payer identifier into the gateway envelope processing flow. The modifications update envelope validation in the publish and sync workers by verifying and recovering the payer address, which is then used to look up or create a payer entry in the database. Corresponding SQL queries, data models, and migration scripts have been updated to add the new Changes
Sequence Diagram(s)sequenceDiagram
participant PW as PublishWorker
participant EV as EnvelopeValidator
participant PE as PayerEnvelope
participant DB as Database
PW->>EV: NewOriginatorEnvelope(originatorEnv)
EV-->>PW: validatedEnvelope
PW->>PE: RecoverSigner() from validatedEnvelope
PE-->>PW: payerAddress
PW->>DB: FindOrCreatePayer(ctx, payerAddress.Hex())
DB-->>PW: payerID
PW->>DB: InsertGatewayEnvelope(..., payerID)
sequenceDiagram
participant SW as SyncWorker
participant PE as PayerEnvelope
participant DB as Database
SW->>PE: Extract payer address from OriginatorEnvelope
PE-->>SW: payerAddress
SW->>DB: FindOrCreatePayer(ctx, payerAddress.Hex())
DB-->>SW: payerID
SW->>DB: Insert envelope with payerID
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/api/message/publishWorker.go (1)
92-96: Consider adding a retry limit or backoff strategy.The infinite retry loop could be problematic if there are persistent validation failures. Consider implementing:
- A maximum retry limit
- An exponential backoff strategy
- A dead letter queue for failed messages
pkg/sync/syncWorker.go (1)
438-442: Enhance error logging for payer ID failures.The error logging could be more descriptive to help with debugging. Consider including the envelope details in the error log.
- s.log.Error("Failed to get payer ID", zap.Error(err)) + s.log.Error("Failed to get payer ID", + zap.Error(err), + zap.Uint32("originatorNodeID", env.OriginatorNodeID()), + zap.Uint64("originatorSequenceID", env.OriginatorSequenceID()))pkg/migrations/00006_link-payer-to-gateway-envelopes.up.sql (1)
1-3: SQL Migration: Adding Nullablepayer_idColumn
The migration file correctly alters thegateway_envelopestable by adding a new nullable columnpayer_idof typeINTwith a foreign key reference topayers(id). The inline comment clarifies that blockchain-originated messages won’t provide apayer_id, which improves clarity.Suggestion: Consider evaluating whether an index on
payer_idmight enhance query performance if future queries filter or join on this column.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
pkg/api/message/publishWorker.go(3 hunks)pkg/api/message/subscribe_test.go(6 hunks)pkg/api/metadata/cursor_test.go(6 hunks)pkg/api/query_test.go(6 hunks)pkg/db/queries.sql(1 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/queries.sql.go(5 hunks)pkg/migrations/00006_link-payer-to-gateway-envelopes.down.sql(1 hunks)pkg/migrations/00006_link-payer-to-gateway-envelopes.up.sql(1 hunks)pkg/sync/syncWorker.go(3 hunks)pkg/testutils/store.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/migrations/00006_link-payer-to-gateway-envelopes.down.sql
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
🔇 Additional comments (9)
pkg/db/queries/models.go (1)
33-33: LGTM!The
PayerIDfield is correctly defined as a nullable integer usingsql.NullInt32.pkg/testutils/store.go (1)
114-127: LGTM!The
CreatePayertest helper function is well-implemented:
- Handles both cases: with and without provided address
- Uses correct address length (42 chars) for random generation
- Properly handles errors
pkg/api/message/publishWorker.go (1)
113-147: LGTM! Robust validation and payer linking implementation.The changes introduce proper:
- Envelope validation
- Payer address recovery
- Database operations for payer linking
- Error handling at each step
pkg/api/metadata/cursor_test.go (1)
31-33: LGTM! Test setup properly integrates payer ID.The changes correctly:
- Create a payer during test setup
- Add payer ID to all test data rows
- Maintain proper cleanup chain
Also applies to: 40-41, 50-51, 61-62, 71-72, 81-82, 89-89
pkg/api/query_test.go (1)
27-28: LGTM! Test setup correctly initializes payer ID.The test setup properly creates a payer and consistently applies the payer ID to all test envelopes, ensuring comprehensive test coverage for the new payer-to-envelope linkage.
pkg/api/message/subscribe_test.go (1)
34-35: LGTM! Subscription tests properly handle payer ID.The test setup correctly initializes and consistently applies the payer ID across all test envelopes, ensuring proper testing of the subscription functionality with the new payer-to-envelope linkage.
Also applies to: 41-41, 51-51, 62-62, 72-72, 82-82
pkg/sync/syncWorker.go (1)
465-478: LGTM! Payer ID recovery and storage logic is correct.The method properly handles:
- Recovering the payer's address from the envelope
- Finding or creating the payer in the database
- Error handling for both operations
pkg/db/queries/queries.sql.go (1)
1-4: LGTM! Generated SQL code properly handles payer ID.This is an auto-generated file by sqlc that correctly implements the database operations for the new payer_id column in the gateway_envelopes table.
Also applies to: 274-279
pkg/db/queries.sql (1)
16-17: SQL Insert Statement Update: Inclusion ofpayer_id
TheINSERT INTO gateway_envelopesstatement has been updated to include the newpayer_idcolumn alongsideoriginator_node_id,originator_sequence_id,topic, andoriginator_envelope. Ensure that the application logic populates the@payer_idparameter appropriately—using a nullable type such assql.NullInt32will handle cases where no payer is provided.
neekolas
force-pushed
the
02-21-link_payer_to_gateway_envelopes
branch
from
9eb284b to
635853b
Compare
neekolas
force-pushed
the
02-21-add_payers_table
branch
2 times, most recently
from
75fb621 to
45e7f32
Compare
neekolas
force-pushed
the
02-21-link_payer_to_gateway_envelopes
branch
from
635853b to
7b21dab
Compare
neekolas
force-pushed
the
02-21-add_payers_table
branch
from
45e7f32 to
75fb621
Compare
neekolas
force-pushed
the
02-21-link_payer_to_gateway_envelopes
branch
from
7b21dab to
eb0b89a
Compare
| @@ -0,0 +1,4 @@ | |||
| ALTER TABLE gateway_envelopes | |||
There was a problem hiding this comment.
nice. so no DB wipe required. Upgrades should be easy. Love it!
neekolas
force-pushed
the
02-21-add_payers_table
branch
from
75fb621 to
3096347
Compare
neekolas
force-pushed
the
02-21-link_payer_to_gateway_envelopes
branch
from
eb0b89a to
ee5308f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/api/message/publishWorker.go (1)
139-147: Consider adding transaction for atomic operations.The payer creation and envelope insertion should ideally be in a transaction to ensure atomicity.
func (p *publishWorker) publishStagedEnvelope(stagedEnv queries.StagedOriginatorEnvelope) bool { // ... existing code ... + tx, err := p.store.BeginTx(p.ctx, nil) + if err != nil { + logger.Error("Failed to begin transaction", zap.Error(err)) + return false + } + defer tx.Rollback() + + q := queries.New(tx) payerId, err := q.FindOrCreatePayer(p.ctx, payerAddress.Hex()) if err != nil { logger.Error("Failed to find or create payer", zap.Error(err)) return false } inserted, err := q.InsertGatewayEnvelope(/* ... */) if err != nil { return false } + if err := tx.Commit(); err != nil { + logger.Error("Failed to commit transaction", zap.Error(err)) + return false + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
pkg/api/message/publishWorker.go(3 hunks)pkg/api/message/subscribe_test.go(6 hunks)pkg/api/metadata/cursor_test.go(6 hunks)pkg/api/query_test.go(6 hunks)pkg/db/queries.sql(1 hunks)pkg/db/queries/models.go(1 hunks)pkg/db/queries/queries.sql.go(5 hunks)pkg/migrations/00006_link-payer-to-gateway-envelopes.down.sql(1 hunks)pkg/migrations/00006_link-payer-to-gateway-envelopes.up.sql(1 hunks)pkg/sync/syncWorker.go(3 hunks)pkg/testutils/envelopes/envelopes.go(2 hunks)pkg/testutils/store.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/migrations/00006_link-payer-to-gateway-envelopes.down.sql
- pkg/migrations/00006_link-payer-to-gateway-envelopes.up.sql
- pkg/db/queries/models.go
- pkg/testutils/store.go
- pkg/db/queries.sql
- pkg/api/message/subscribe_test.go
- pkg/sync/syncWorker.go
- pkg/db/queries/queries.sql.go
- pkg/api/query_test.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
🔇 Additional comments (5)
pkg/testutils/envelopes/envelopes.go (2)
6-12: LGTM!The import reordering improves readability by grouping related imports together.
84-108:❓ Verification inconclusive
Verify impact of removing TargetOriginator.
The removal of
TargetOriginatorfield fromPayerEnvelopemight affect test cases that depend on this field.
🏁 Script executed:
#!/bin/bash # Search for test cases that might expect TargetOriginator in PayerEnvelope rg -A 5 "CreatePayerEnvelope.*\n.*TargetOriginator"Length of output: 245
Action Required: Verify Impact of Removing TargetOriginator
The updated search—with multiline mode enabled—did not reveal any test cases referencing the removed
TargetOriginatorfield in the context ofCreatePayerEnvelope. Although no dependencies were found, please double-check that:
- No tests (or other parts of the codebase) expect the
TargetOriginatorfield in instances ofPayerEnvelope.- All related tests pass after this field removal.
If you identify additional dependencies or notice test failures, further investigation may be needed.
File: pkg/testutils/envelopes/envelopes.go, Lines: 84-108
func CreatePayerEnvelope( t *testing.T, nodeID uint32, clientEnv ...*envelopes.ClientEnvelope, ) *envelopes.PayerEnvelope { if len(clientEnv) == 0 { clientEnv = append(clientEnv, CreateClientEnvelope()) } clientEnvBytes, err := proto.Marshal(clientEnv[0]) require.NoError(t, err) key, err := crypto.GenerateKey() require.NoError(t, err) payerSignature, err := utils.SignClientEnvelope(nodeID, clientEnvBytes, key) require.NoError(t, err) return &envelopes.PayerEnvelope{ UnsignedClientEnvelope: clientEnvBytes, PayerSignature: &associations.RecoverableEcdsaSignature{ Bytes: payerSignature, }, } }Please perform a manual review to confirm these findings.
🏁 Script executed:
#!/bin/bash # Re-run grep in multiline mode to search for any test cases referencing 'TargetOriginator' # in the context of CreatePayerEnvelope. This should capture cases spanning multiple lines. rg -U --multiline -A 5 "CreatePayerEnvelope.*\n.*TargetOriginator" .Length of output: 70
Manual Verification Required: Confirm Removal of
TargetOriginatorHas No Side EffectsThe automated search for references to
TargetOriginator—especially in relation to theCreatePayerEnvelopefunction—did not produce any matches. This indicates that no tests or other parts of the codebase appear to rely on this field. However, please manually verify that:
- No test cases expect the
TargetOriginatorfield withinPayerEnvelope.- All tests are passing after this removal.
- There are no hidden dependencies or indirect usages elsewhere in the codebase.
File: pkg/testutils/envelopes/envelopes.go, Lines: 84-108
func CreatePayerEnvelope( t *testing.T, nodeID uint32, clientEnv ...*envelopes.ClientEnvelope, ) *envelopes.PayerEnvelope { if len(clientEnv) == 0 { clientEnv = append(clientEnv, CreateClientEnvelope()) } clientEnvBytes, err := proto.Marshal(clientEnv[0]) require.NoError(t, err) key, err := crypto.GenerateKey() require.NoError(t, err) payerSignature, err := utils.SignClientEnvelope(nodeID, clientEnvBytes, key) require.NoError(t, err) return &envelopes.PayerEnvelope{ UnsignedClientEnvelope: clientEnvBytes, PayerSignature: &associations.RecoverableEcdsaSignature{ Bytes: payerSignature, }, } }Please conduct a manual review to ensure that no further code or test expectations include the removed
TargetOriginatorfield.pkg/api/message/publishWorker.go (2)
113-118: LGTM!The addition of envelope validation using
NewOriginatorEnvelopeimproves robustness by ensuring envelope integrity before processing.
126-136: LGTM!The implementation correctly recovers the payer's address from the envelope signature and creates/finds the payer record in the database. The error handling is comprehensive.
pkg/api/metadata/cursor_test.go (1)
28-90: LGTM!The changes correctly integrate payer tracking into the test setup:
- The function now returns all necessary components (api, db, mocks, cleanup)
- Payer creation is properly handled using
CreatePayer- Test data consistently includes payer ID across all test cases
neekolas
force-pushed
the
02-21-link_payer_to_gateway_envelopes
branch
from
ee5308f to
8d666c7
Compare
TL;DR
Added payer tracking to gateway envelopes by linking them to payer records in the database.
It's very hard to fill this link in after-the-fact, since it requires recovering the signer address from the
PayerEnvelope, so I think it makes sense to store it on every row even if we don't have an immediate use for querying by the field.Issues
What changed?
payer_idcolumn to thegateway_envelopestable that references thepayerstableInsertGatewayEnvelopequery to include thepayer_idfieldHow to test?
Why make this change?
This change enables tracking of message payers in the system, which is essential for:
Summary by CodeRabbit
New Features
Chores