Allow running without payer key #547
Conversation
WalkthroughThis pull request removes the external initialization of the blockchain signer and publisher from the replication entry point. The changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant ReplicationServer
participant API
participant PayerService
participant Signer
participant BlockchainClient
participant BlockchainPublisher
Main->>ReplicationServer: Initialize replication server (without blockchainPublisher)
ReplicationServer->>API: Call startAPIServer(...)
API->>PayerService: Initialize payer service
API->>Signer: Create signer (using private key & chain ID)
Signer-->>API: Return signer
API->>BlockchainClient: Connect using RPC URL
BlockchainClient-->>API: Return blockchain client
API->>BlockchainPublisher: Initialize publisher using signer & client
BlockchainPublisher-->>API: Return blockchain publisher
API->>PayerService: Configure payer service with blockchain publisher
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24) ✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
pkg/server/server_test.go (1)
132-133:⚠️ Potential issueFix inconsistency in test function calls.
The
NewTestServerfunction signature no longer accepts theprivateKeyparameter, but it's still being passed in the test cases.Apply this diff to fix the test cases:
- server1 := NewTestServer(t, server1Port, dbs[0], registry, privateKey1) - server2 := NewTestServer(t, server2Port, dbs[1], registry, privateKey2) + server1 := NewTestServer(t, server1Port, dbs[0], registry) + server2 := NewTestServer(t, server2Port, dbs[1], registry) - server1 := NewTestServer(t, server1Port, dbs[0], registry, privateKey1) + server1 := NewTestServer(t, server1Port, dbs[0], registry)Also applies to: 246-246
🧹 Nitpick comments (1)
pkg/server/server_test.go (1)
43-49: Update function signature documentation.The function signature has been updated to remove the
privateKeyparameter, but the documentation should be updated to reflect this change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/replication/main.go(0 hunks)pkg/server/server.go(1 hunks)pkg/server/server_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- cmd/replication/main.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
🔇 Additional comments (1)
pkg/server/server.go (1)
219-247: LGTM! The changes align with the PR objective.Moving the blockchain publisher initialization inside the payer service block makes the payer key optional, which addresses the issue where XMTP API/SYNC crashes if the payer key is not provided.
| signer, err := blockchain.NewPrivateKeySigner( | ||
| options.Payer.PrivateKey, | ||
| options.Contracts.ChainID, | ||
| ) | ||
| if err != nil { | ||
| log.Fatal("initializing signer", zap.Error(err)) | ||
| } | ||
|
|
||
| ethclient, err := blockchain.NewClient(ctx, options.Contracts.RpcUrl) | ||
| if err != nil { | ||
| log.Fatal("initializing blockchain client", zap.Error(err)) | ||
| } | ||
|
|
||
| blockchainPublisher, err := blockchain.NewBlockchainPublisher( | ||
| ctx, | ||
| log, | ||
| ethclient, | ||
| signer, | ||
| options.Contracts, | ||
| ) | ||
| if err != nil { | ||
| log.Fatal("initializing message publisher", zap.Error(err)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using graceful error handling instead of log.Fatal.
The current implementation uses log.Fatal for error handling, which immediately terminates the program. Consider returning the errors to allow graceful shutdown and proper cleanup.
Apply this diff to improve error handling:
signer, err := blockchain.NewPrivateKeySigner(
options.Payer.PrivateKey,
options.Contracts.ChainID,
)
if err != nil {
- log.Fatal("initializing signer", zap.Error(err))
+ log.Error("initializing signer", zap.Error(err))
+ return err
}
ethclient, err := blockchain.NewClient(ctx, options.Contracts.RpcUrl)
if err != nil {
- log.Fatal("initializing blockchain client", zap.Error(err))
+ log.Error("initializing blockchain client", zap.Error(err))
+ return err
}
blockchainPublisher, err := blockchain.NewBlockchainPublisher(
ctx,
log,
ethclient,
signer,
options.Contracts,
)
if err != nil {
- log.Fatal("initializing message publisher", zap.Error(err))
+ log.Error("initializing message publisher", zap.Error(err))
+ return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| signer, err := blockchain.NewPrivateKeySigner( | |
| options.Payer.PrivateKey, | |
| options.Contracts.ChainID, | |
| ) | |
| if err != nil { | |
| log.Fatal("initializing signer", zap.Error(err)) | |
| } | |
| ethclient, err := blockchain.NewClient(ctx, options.Contracts.RpcUrl) | |
| if err != nil { | |
| log.Fatal("initializing blockchain client", zap.Error(err)) | |
| } | |
| blockchainPublisher, err := blockchain.NewBlockchainPublisher( | |
| ctx, | |
| log, | |
| ethclient, | |
| signer, | |
| options.Contracts, | |
| ) | |
| if err != nil { | |
| log.Fatal("initializing message publisher", zap.Error(err)) | |
| } | |
| signer, err := blockchain.NewPrivateKeySigner( | |
| options.Payer.PrivateKey, | |
| options.Contracts.ChainID, | |
| ) | |
| if err != nil { | |
| log.Error("initializing signer", zap.Error(err)) | |
| return err | |
| } | |
| ethclient, err := blockchain.NewClient(ctx, options.Contracts.RpcUrl) | |
| if err != nil { | |
| log.Error("initializing blockchain client", zap.Error(err)) | |
| return err | |
| } | |
| blockchainPublisher, err := blockchain.NewBlockchainPublisher( | |
| ctx, | |
| log, | |
| ethclient, | |
| signer, | |
| options.Contracts, | |
| ) | |
| if err != nil { | |
| log.Error("initializing message publisher", zap.Error(err)) | |
| return err | |
| } |
If you were to try to run the XMTP API/SYNC without providing the payer
key, the node will crash:
```
$ ./dev/run
2025-02-24T14:01:43.485-0500 INFO replication Version: v0.2.0-9-g1f8192f
2025-02-24T14:01:43.495-0500 INFO replication Successfully connected to DB {"namespace": "xmtpd_c04b2306614f"}
2025-02-24T14:01:43.512-0500 FATAL replication initializing signer {"error": "unable to parse private key: invalid length, need 256 bits"}
exit status 1
```
The blockchain publisher is only required for the payer. If we ever need
it for the node, it will have to use the node key.
I will break apart the XMTPD image and the payer image in the next few
days. But I need this to get the infra code moving.
If you were to try to run the XMTP API/SYNC without providing the payer key, the node will crash:
The blockchain publisher is only required for the payer. If we ever need it for the node, it will have to use the node key.
I will break apart the XMTPD image and the payer image in the next few days. But I need this to get the infra code moving.
Summary by CodeRabbit
Refactor
Tests