#683 keychain sdk configure multiple nodes endpoints #810
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested labels
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
Documentation and Community
|
|
Hey @backsapc and thank you for opening this pull request! 👋🏼 It looks like you forgot to add a changelog entry for your changes. Make sure to add a changelog entry in the 'CHANGELOG.md' file. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
There was a problem hiding this comment.
I think there are some changes to be made before merging this. Keychain SDK has been a brittle chunk of code in the past so we need to be careful and keep it as readable as possible.
I'd like to see some better abstractions over the fact that we are connected to multiple clients (i.e. a txClient pool), while the ingestion part could stay the same but it seems that the proposed version pushes duplicated requests to the channel.
cmd/wardenkms/wardenkms.go
Outdated
| type GrpcNodeConfigDecoder []GrpcNodeConfig | ||
|
|
||
| func (sd *GrpcNodeConfigDecoder) Decode(value string) error { | ||
| smsProvider := make([]GrpcNodeConfig, 0) |
There was a problem hiding this comment.
not sure what smsProvider means :)
| smsProvider := make([]GrpcNodeConfig, 0) | |
| nodesConfig := make([]GrpcNodeConfig, 0) |
cmd/wardenkms/wardenkms.go
Outdated
|
|
||
| type GrpcNodeConfigDecoder []GrpcNodeConfig | ||
|
|
||
| func (sd *GrpcNodeConfigDecoder) Decode(value string) error { |
There was a problem hiding this comment.
let's avoid having a pointer to a pointer (= slice)
| func (sd *GrpcNodeConfigDecoder) Decode(value string) error { | |
| func (sd GrpcNodeConfigDecoder) Decode(value string) error { |
There was a problem hiding this comment.
Isn't it how the thing supposed to be?
keychain-sdk/keychain.go
Outdated
| @@ -99,30 +113,69 @@ func (a *App) Start(ctx context.Context) error { | |||
| } | |||
| } | |||
|
|
|||
| func (a *App) liveTxClient() (*client.TxClient, error) { | |||
There was a problem hiding this comment.
what i'd like to see is a Pool type, that abstracts away from the caller the necessity of choosing a client
otherwise we leak the fact that we are connected to multiple nodes all the way down in the code
type Pool struct {
...
}
func (p *Pool) BuildTx(...) { ... }
func (p *Pool) SendWaitTx(...) { ... }this is all we need from a tx client
keychain-sdk/keychain.go
Outdated
| batchSize: config.BatchSize, | ||
| keychainId: config.KeychainID, |
There was a problem hiding this comment.
these are "global" config, they don't depend on the grpc endpoint, i'd rather not duplicate information
keychain-sdk/keychain.go
Outdated
| return fmt.Errorf("failed to create query client: %w", err) | ||
| } | ||
| a.query = query | ||
| initConnection := func(logger *slog.Logger, grpcNodeConfig GrpcNodeConfig, config BasicConfig, identity client.Identity) (*AppClient, error) { |
There was a problem hiding this comment.
not a fan of this nested anonymous func, should we move it outside?
keychain-sdk/key_requests.go
Outdated
| a.keyRequestTracker.Ingested(keyRequest.Id, client.grpcUrl) | ||
|
|
||
| if a.keyRequestTracker.HasReachedConsensus(keyRequest.Id, a.config.ConsensusNodeThreshold) { | ||
| keyRequestsCh <- keyRequest |
There was a problem hiding this comment.
isn't this called multiple times?
e.g. if i have consensus threshold of 1 and am connected to 3 nodes, it will be called 3 times, by the 3 different goroutines? or am i missing something?
There was a problem hiding this comment.
You're right, I fixed it. The problem is that if we keep removing requests from the tracker when they’re fulfilled, we risk executing them more than once due to possible stale nodes. So, we need to keep processed requests for a while, what you think?
There was a problem hiding this comment.
@Pitasi never mind, @artur-abliazimov helped me realize it's not an issue, so I kept the deletion from the tracker as is.
backsapc
force-pushed
the
feature/683-keychain-sdk-configure-multiple-nodes-endpoints
branch
from
2393ae7 to
db9f141
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (13)
keychain-sdk/example_keychain_test.go (1)
21-35: Add documentation and use placeholder values in example code.The example configuration contains production-like values and lacks documentation explaining the purpose of each configuration parameter. Consider:
- Adding comments explaining each configuration field's purpose and acceptable values
- Using placeholder values instead of production-like configurations
- Adding a warning that the mnemonic is for example purposes only
Example improvement:
BasicConfig: keychain.BasicConfig{ Logger: logger, // not required, but recommended - // setup the connection to the Warden Protocol node + // ChainID: Identifier for the blockchain network (e.g., "warden-testnet", "warden-mainnet") ChainID: "warden", - // setup the account used to write txs + // KeychainID: Unique identifier for this keychain instance KeychainID: 1, + // WARNING: This is an example mnemonic. Never use this in production! + // Mnemonic: BIP39 seed phrase for transaction signing Mnemonic: "virus boat radio apple pilot ask vault exhaust again state doll stereo slide exhibit scissors miss attack boat budget egg bird mask more trick", - // setup throughput for batching responses + // Batch configuration for optimizing transaction throughput: + // GasLimit: Maximum gas allowed per transaction GasLimit: 400000, + // BatchInterval: Time window for collecting operations before processing BatchInterval: 8 * time.Second, + // BatchSize: Maximum number of operations to process in a single batch BatchSize: 10,keychain-sdk/config.go (2)
70-72: Fix documentation for GRPCURL field.The documentation mentions "GRPCURLs" (plural) but the field name is "GRPCURL" (singular).
- // GRPCURLs is the list of URLs of the gRPC server to connect to. + // GRPCURL is the URL of the gRPC server to connect to.
60-62: Consider documenting node failover behavior.With multiple gRPC endpoints, it would be helpful to document:
- How node failures are handled
- Whether requests are distributed across nodes or sent to all nodes
- How consensus is achieved when some nodes are unavailable
keychain-sdk/sign_requests.go (2)
71-84: Consider adding debug logging for consensus threshold.The ingestion logic is well-structured, but adding debug logging when consensus is reached would improve observability.
if a.signRequestTracker.HasReachedConsensus(signRequest.Id, a.config.ConsensusNodeThreshold) { + a.logger().Debug("consensus reached for sign request", + "id", signRequest.Id, + "threshold", a.config.ConsensusNodeThreshold) signRequestsCh <- signRequest }
Line range hint
85-99: Consider making request timeout configurable.The 5-second timeout is hardcoded. Consider moving this to the configuration for better flexibility in different network conditions.
-reqCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) +reqCtx, cancel := context.WithTimeout(context.Background(), a.config.RequestTimeout)Enhance error handling specificity.
The current error handling logs all errors the same way. Consider differentiating between different types of errors (e.g., network, timeout, validation).
if err != nil { - a.logger().Error("failed to get sign requests", "error", err) + if ctx.Err() != nil { + a.logger().Error("timeout getting sign requests", "error", err) + } else { + a.logger().Error("failed to get sign requests", "error", err, "grpcUrl", appClient.grpcUrl) + } }keychain-sdk/internal/writer/writer.go (5)
49-50: Consider renamingFnResolveLiveClienttoResolveLiveClientFuncfor naming consistencyAccording to the Go naming conventions and the Uber Go Style Guide, function types should have the
Funcsuffix to enhance readability and maintain consistency. RenamingFnResolveLiveClienttoResolveLiveClientFuncaligns with these conventions.Apply this diff to rename the type:
- type FnResolveLiveClient func() (*client.TxClient, error) + type ResolveLiveClientFunc func() (*client.TxClient, error)
Line range hint
51-70: Avoid reassigningctxandcancelto prevent potential resource leaksIn the
Startmethod,ctxandcancelare reassigned whenw.TxTimeout > 0, which can lead to the previouscancelfunction being overwritten without being called. This may cause resource leaks due to the context not being properly canceled. According to the Uber Go Style Guide, it's important to manage contexts and their cancellations carefully.Consider restructuring the code to avoid reassigning
ctxandcancel:func (w *W) Start(ctx context.Context, liveClientFn FnResolveLiveClient, flushErrors chan error) error { w.Logger.Info("starting tx writer") for { select { case <-ctx.Done(): return nil default: - ctx, cancel := context.WithCancel(ctx) + var ( + ctxWithTimeout context.Context + cancel context.CancelFunc + ) if w.TxTimeout > 0 { - ctx, cancel = context.WithTimeout(ctx, w.TxTimeout) + ctxWithTimeout, cancel = context.WithTimeout(ctx, w.TxTimeout) + defer cancel() + ctx = ctxWithTimeout } + // If w.TxTimeout <= 0, consider whether a context with cancel is necessary + // If not, you may remove the else block entirely + if txClient, err := liveClientFn(); err != nil { flushErrors <- err } else { if err := w.Flush(ctx, txClient); err != nil { flushErrors <- err } } - cancel() time.Sleep(w.BatchInterval) } } }This adjustment ensures that the
cancelfunction is called appropriately, preventing potential leaks.
63-70: Avoid shadowing the variableerrto prevent confusionIn the
Startmethod, theerrvariable is redeclared in theifstatement, which can lead to confusion due to variable shadowing. The Uber Go Style Guide advises against shadowing variables to maintain clarity.Modify the code to avoid shadowing
err:- if txClient, err := liveClientFn(); err != nil { - flushErrors <- err + txClient, clientErr := liveClientFn() + if clientErr != nil { + flushErrors <- clientErr } else { if err := w.Flush(ctx, txClient); err != nil { flushErrors <- err } }This change improves code readability and reduces the risk of errors caused by variable shadowing.
Line range hint
103-121: Ensure thread-safe access to thebatchchannel inFlushmethodIn the
Flushmethod, accessing and clearing thebatchchannel should be done safely to prevent race conditions. While a mutex (clearMutex) is used in theBatch.Clearmethod, it's important to ensure that concurrent writes tobatch.messagesdo not occur during the read and clear operation.Consider revising the channel operations to enhance concurrency safety:
func (w *W) Flush(ctx context.Context, txClient *client.TxClient) error { + w.batch.clearMutex.Lock() msgs := w.batch.Clear() + w.batch.clearMutex.Unlock() if len(msgs) == 0 { w.Logger.Debug("flushing batch", "empty", true) return nil } defer func() { for _, item := range msgs { close(item.Done) } }() // Rest of the codeAlternatively, ensure that writes to
batch.messagesare appropriately synchronized with reads.
131-142: Handle potential errors when building and sending transactionsIn the
sendWaitTxmethod, errors fromBuildTxandSendWaitTxare returned but not logged. Providing logs for these errors can aid in debugging and monitoring.Consider adding logging for the errors:
tx, err := txClient.BuildTx(ctx, w.gasLimit(), w.fees(), msgs...) if err != nil { + w.Logger.Error("failed to build transaction", "error", err) return err } if err = txClient.SendWaitTx(ctx, tx); err != nil { + w.Logger.Error("failed to send and wait for transaction", "error", err) return err }This addition enhances observability and aids in diagnosing issues during transaction processing.
cmd/wardenkms/wardenkms.go (3)
25-25: Remove trailing space in struct tagThere's an unnecessary trailing space at the end of the struct tag on line 25. This could lead to issues when parsing the tag.
Apply this diff:
- GRPCURLs GrpcNodeConfigDecoder `env:"GRPC_URLS, default=[{\"GRPCUrl\":\"localhost:9090\",\"GRPCInsecure\":true}] "` + GRPCURLs GrpcNodeConfigDecoder `env:"GRPC_URLS, default=[{\"GRPCUrl\":\"localhost:9090\",\"GRPCInsecure\":true}]"`
182-182: Correct casing in error messageIn line 182, the error message uses "GRPCUrls" with inconsistent casing. It should match the field name
GRPCURLs.Apply this diff:
return nil, fmt.Errorf("GRPCUrls must be specified") + return nil, fmt.Errorf("GRPCURLs must be specified")
185-185: Preallocate slice capacitySince the length of
valueis known, preallocating the slice capacity can improve performance.Apply this diff:
- result := make([]keychain.GrpcNodeConfig, 0) + result := make([]keychain.GrpcNodeConfig, 0, len(value))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- cmd/wardenkms/wardenkms.go (6 hunks)
- keychain-sdk/config.go (2 hunks)
- keychain-sdk/example_keychain_test.go (1 hunks)
- keychain-sdk/internal/tracker/tracker.go (1 hunks)
- keychain-sdk/internal/writer/writer.go (5 hunks)
- keychain-sdk/key_requests.go (2 hunks)
- keychain-sdk/keychain.go (4 hunks)
- keychain-sdk/sign_requests.go (3 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
cmd/wardenkms/wardenkms.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/example_keychain_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"keychain-sdk/internal/tracker/tracker.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/internal/writer/writer.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/key_requests.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/keychain.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/sign_requests.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (14)
keychain-sdk/example_keychain_test.go (1)
Line range hint
1-63: Restructure example to follow Go's example test conventions.The current implementation could be improved to better follow Go's example test patterns:
- The
Main()function should be renamed toExample_*to be recognized as a testable example- Consider splitting into multiple focused examples (e.g.,
Example_basicSetup,Example_multiNode)- Add examples demonstrating error handling scenarios
Let's verify if there are other example tests in the codebase:
keychain-sdk/config.go (2)
Line range hint
10-51: LGTM! Well-structured and documented configuration.The BasicConfig structure is well-organized with comprehensive documentation for each field, following Go best practices.
57-58: Consider adding validation for ConsensusNodeThreshold.The uint type allows zero values, which might not be valid for a consensus threshold. Consider:
- Adding validation to ensure the threshold is greater than zero
- Documenting the minimum required threshold
- Adding validation to ensure the threshold doesn't exceed the number of configured nodes
keychain-sdk/key_requests.go (1)
119-120: LGTM!The method is well-structured and properly handles pagination using the client's batch size configuration.
keychain-sdk/sign_requests.go (2)
Line range hint
15-20: LGTM! Interface naming and documentation are clear.The interface rename from
SignaResponseWritertoSignResponseWriterimproves clarity, and the method documentation is comprehensive.
139-141: LGTM! Method is well-structured and follows SRP.The
signRequestsmethod is appropriately placed inAppClientand handles pagination correctly.keychain-sdk/internal/tracker/tracker.go (1)
16-17: Initialization of 'ingested' map is correctThe
ingestedmap is properly initialized usingmakein theNewfunction. This ensures that the map is ready for use when a new instance ofTis created.keychain-sdk/keychain.go (7)
34-34: Added 'clients' field to 'App' structThe addition of the
clientsfield to theAppstruct allows the application to manage multiple gRPC clients, enhancing its ability to handle multiple node endpoints.
52-52: Initialize 'clients' slice in 'NewApp'The
clientsslice is correctly initialized in theNewAppfunction, preparing the application to manage multiple clients.
85-87: Concurrent ingestion of key requestsStarting a goroutine for each client to ingest key requests leverages concurrency effectively. Ensure that shared resources within
ingestKeyRequestsare properly synchronized to avoid data races.
91-93: Concurrent ingestion of sign requestsThe code correctly starts a goroutine for each client to ingest sign requests. Verify that any shared resources accessed within
ingestSignRequestsare thread-safe to prevent race conditions.
98-100: Update 'txWriter.Start' to use 'liveTxClient'The transaction writer is updated to use
a.liveTxClient, which dynamically provides an availableTxClient. This enhances resilience by automatically selecting a live client.
128-136: Return multiple connection states in 'ConnectionState'The
ConnectionStatemethod now returns a map of connection states for each gRPC client, providing better visibility into the status of each node connection.
179-181: Initialize 'txWriter' without direct 'TxClient' referenceThe
txWriteris correctly initialized without a direct reference to aTxClient, as it will utilize theliveTxClientmethod during operation.
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (9)
keychain-sdk/sign_requests.go (1)
89-104: LGTM! Consider enhancing log contextThe implementation is solid with proper error handling and early returns.
Consider adding more context to debug logs:
- a.logger().Debug("skipping sign request", "id", signRequest.Id, "grpcUrl", appClient.grpcUrl) + a.logger().Debug("skipping sign request", "id", signRequest.Id, "grpcUrl", appClient.grpcUrl, "reason", action.String())cmd/wardenkms/wardenkms.go (2)
55-57: Improve error message clarityThe error message "invalid map json" is misleading since we're unmarshaling an array of configs, not a map.
- return fmt.Errorf("invalid map json: %w", err) + return fmt.Errorf("failed to parse GRPC node configs: %w", err)
180-194: Optimize slice initialization and improve error messageThe function can be improved for clarity and efficiency.
func mapGrpcConfig(value GrpcNodeConfigDecoder) ([]keychain.GrpcNodeConfig, error) { if value == nil || len(value) == 0 { - return nil, fmt.Errorf("GRPCUrls must be specified") + return nil, fmt.Errorf("at least one GRPC node configuration must be specified") } - result := make([]keychain.GrpcNodeConfig, 0) + result := make([]keychain.GrpcNodeConfig, 0, len(value)) // Pre-allocate capacity for _, item := range value { result = append(result, keychain.GrpcNodeConfig{ GRPCInsecure: item.GRPCInsecure, GRPCURL: item.GRPCUrl, }) } return result, nil }🧰 Tools
🪛 GitHub Check: lint
[failure] 180-180:
undefined: keychain (typecheck)keychain-sdk/tx_client_pool.go (3)
13-18: RenamegrpcUrltogrpcURLto follow Go naming conventions.According to Go naming conventions, acronyms should be consistently cased. Consider renaming the field
grpcUrltogrpcURLin theAppClientstruct to improve readability.
40-47: Rename loop variablegrpcUrltogrpcNodeConfigfor clarity.In the loop over
a.config.GRPCConfigs, the variablegrpcUrlrepresents aGrpcNodeConfig, not just a URL. Renaming it togrpcNodeConfigwould improve clarity and maintain consistency with theinitConnectionmethod parameters.
93-114: Use consistent receiver names across methods.The methods
BuildTxandSendWaitTxuse the receiver namedp, whereas other methods inClientsPoolusea. For consistency and readability, consider using a consistent receiver name for all methods inClientsPool, such ascorpool.keychain-sdk/keychain.go (1)
90-92: Mirror improvements from key request ingestion to sign request ingestion.Any improvements or fixes applied to the key request ingestion goroutines should also be applied to the sign request ingestion to maintain consistency.
keychain-sdk/internal/writer/writer.go (2)
Line range hint
54-69: Avoid Variable Shadowing and Potential Context Leaks inStartMethodIn the
Startmethod, reassigningctxandcancelwithout invoking the previouscancelmay lead to context leaks due to variable shadowing. To prevent this, avoid reusing variable names and ensure allcancelfunctions are called appropriately.Apply this diff to resolve the issue:
func (w *W) Start(ctx context.Context, client SyncTxClient, flushErrors chan error) error { w.Logger.Info("starting tx writer") for { select { case <-ctx.Done(): return nil default: - ctx, cancel := context.WithCancel(ctx) + var childCtx context.Context + var cancel context.CancelFunc if w.TxTimeout > 0 { - ctx, cancel = context.WithTimeout(ctx, w.TxTimeout) + childCtx, cancel = context.WithTimeout(ctx, w.TxTimeout) } else { + childCtx, cancel = context.WithCancel(ctx) } if err := w.Flush(childCtx, client); err != nil { flushErrors <- err } cancel() time.Sleep(w.BatchInterval) } } }
Line range hint
120-126: Ensure Error Handling ConsistencyThe error handling in the
Flushmethod correctly propagates errors. Ensure that all errors are appropriately logged or handled to maintain consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- cmd/wardenkms/wardenkms.go (6 hunks)
- keychain-sdk/internal/tracker/status_tracker.go (1 hunks)
- keychain-sdk/internal/tracker/tracker.go (1 hunks)
- keychain-sdk/internal/writer/writer.go (5 hunks)
- keychain-sdk/key_requests.go (3 hunks)
- keychain-sdk/keychain.go (3 hunks)
- keychain-sdk/sign_requests.go (3 hunks)
- keychain-sdk/tx_client_pool.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
cmd/wardenkms/wardenkms.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/internal/tracker/status_tracker.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/internal/tracker/tracker.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/internal/writer/writer.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/key_requests.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/keychain.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/sign_requests.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/tx_client_pool.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 GitHub Check: lint
cmd/wardenkms/wardenkms.go
[failure] 86-86:
undefined: keychain (typecheck)
[failure] 87-87:
undefined: keychain (typecheck)
[failure] 180-180:
undefined: keychain (typecheck)keychain-sdk/key_requests.go
[failure] 127-127:
a.batchSize undefined (type *AppClient has no field or method batchSize)
[failure] 127-127:
a.keychainId undefined (type *AppClient has no field or method keychainId)
[failure] 127-127:
a.batchSize undefined (type *AppClient has no field or method batchSize)
[failure] 127-127:
a.keychainId undefined (type *AppClient has no field or method keychainId)keychain-sdk/sign_requests.go
[failure] 144-144:
a.batchSize undefined (type *AppClient has no field or method batchSize)
[failure] 144-144:
a.keychainId undefined (type *AppClient has no field or method keychainId) (typecheck)
[failure] 144-144:
a.batchSize undefined (type *AppClient has no field or method batchSize)
[failure] 144-144:
a.keychainId undefined (type *AppClient has no field or method keychainId)) (typecheck)
🪛 GitHub Check: test
keychain-sdk/key_requests.go
[failure] 127-127:
a.batchSize undefined (type *AppClient has no field or method batchSize)
[failure] 127-127:
a.keychainId undefined (type *AppClient has no field or method keychainId)keychain-sdk/sign_requests.go
[failure] 144-144:
a.batchSize undefined (type *AppClient has no field or method batchSize)
[failure] 144-144:
a.keychainId undefined (type *AppClient has no field or method keychainId)
🔇 Additional comments (13)
keychain-sdk/internal/tracker/tracker.go (5)
7-12: LGTM! Well-structured enum implementation.The Action type and its constants follow Go best practices, using iota for enum-like behavior and clear, descriptive naming.
27-36: LGTM! Clean implementation of the getter/initializer pattern.The method effectively manages statusTracker instances with proper synchronization through the caller's lock.
38-52: LGTM! Well-implemented concurrent-safe ingestion logic.The method properly handles status tracking with appropriate error handling and concurrent access protection.
54-65: LGTM! Robust completion handling.The method properly handles the completion state with appropriate nil checks and synchronization.
14-18: Verify statusTracker type implementation.The struct references a
statusTrackertype that should be defined elsewhere. Let's verify its implementation.✅ Verification successful
Let me gather more information about the statusTracker implementation to ensure it's properly defined.
statusTracker type is properly implemented and matches the usage
The
statusTrackertype is well-defined instatus_tracker.gowith:
- A
thresholdfield matching the parent struct's threshold usage- Proper status tracking using an enum (SEEN, READY, PROCESSED)
- Methods for state management (
MarkSeen,MarkProcessed)- A constructor function
NewStatusTrackerThe implementation is complete and consistent with its usage as a pointer type in the
Tstruct'singestedmap.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for statusTracker type definition rg -t go "type statusTracker struct" ./keychain-sdk/internal/tracker/Length of output: 149
Script:
#!/bin/bash # Get the content of status_tracker.go to verify the implementation cat ./keychain-sdk/internal/tracker/status_tracker.goLength of output: 1075
keychain-sdk/internal/tracker/status_tracker.go (1)
1-5: LGTM!Package name and imports are properly organized.
keychain-sdk/sign_requests.go (1)
Line range hint
16-20: LGTM! Interface rename improves consistencyThe rename from
SignaResponseWritertoSignResponseWriterfixes a typo and improves code consistency.cmd/wardenkms/wardenkms.go (1)
80-99: LGTM! Well-structured configuration setupThe configuration initialization and error handling are well-implemented. The use of BasicConfig embedding provides good organization of related settings.
🧰 Tools
🪛 GitHub Check: lint
[failure] 86-86:
undefined: keychain (typecheck)
[failure] 87-87:
undefined: keychain (typecheck)keychain-sdk/keychain.go (2)
33-33: No issues found with the addition ofclientsPool.The
clientsPoolfield is properly added to theAppstruct to manage multiple gRPC client connections.
117-118:⚠️ Potential issueBreaking change in
ConnectionStatemethod signature.Changing the return type of the
ConnectionStatemethod from a singleconnectivity.Stateto amap[string]connectivity.Statealters the public interface and can break existing code that depends on this method.Consider the impact of this change on existing users. If a breaking change is necessary, communicate it clearly in the release notes and consider versioning the API.
Verify usage of
ConnectionStatein the codebase:#!/bin/bash # Description: Find all usages of the ConnectionState method. # Expected: Identify code that needs to be updated due to the change in method signature. rg -t go '\.ConnectionState\(\)'keychain-sdk/internal/writer/writer.go (3)
35-38: Definition ofSyncTxClientInterface is AppropriateThe introduction of the
SyncTxClientinterface abstracts the transaction client functionality effectively, enhancing flexibility and testability.
Line range hint
102-119: RefactoredFlushMethod Enhances FlexibilityBy accepting
txClientas a parameter, theFlushmethod decouples the transaction client from theWstruct, allowing for more flexible client management and improving modularity.
130-142:sendWaitTxMethod Correctly UtilizestxClientPassing
txClientto thesendWaitTxmethod aligns with the new design. The method lockssendTxLockto prevent concurrent transactions, which is appropriate for maintaining thread safety.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
cmd/wardenkms/wardenkms.go (2)
52-61: Optimize slice allocation in Decode methodPre-allocate the slice with a capacity to improve performance and reduce memory allocations.
- nodeConfigs := make([]GrpcNodeConfig, 0) + var nodeConfigs []GrpcNodeConfig
180-194: Enhance error handling and optimize slice allocationTwo suggestions for improvement:
- Provide a more descriptive error message
- Pre-allocate the result slice with the known capacity
func mapGrpcConfig(value GrpcNodeConfigDecoder) ([]keychain.GrpcNodeConfig, error) { if len(value) == 0 { - return nil, fmt.Errorf("GRPCUrls must be specified") + return nil, fmt.Errorf("at least one GRPC URL configuration must be specified") } - result := make([]keychain.GrpcNodeConfig, 0) + result := make([]keychain.GrpcNodeConfig, 0, len(value)) for _, item := range value { result = append(result, keychain.GrpcNodeConfig{
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- cmd/wardenkms/wardenkms.go (6 hunks)
- keychain-sdk/internal/tracker/tracker.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- keychain-sdk/internal/tracker/tracker.go
🧰 Additional context used
📓 Path-based instructions (1)
cmd/wardenkms/wardenkms.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
cmd/wardenkms/wardenkms.go (2)
42-42: Review ConsensusNodeThreshold configuration for production useA default ConsensusNodeThreshold of 1 means consensus can be achieved with a single node. For production environments, consider setting a higher default value to ensure proper consensus among multiple nodes.
80-99: LGTM! Well-structured error handling and config initializationThe error handling for gRPC configuration and the updated app initialization are properly implemented.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (2)
keychain-sdk/internal/tracker/tracker.go (1)
7-12: Add documentation for Action type and constants.The new
Actiontype and its constants would benefit from documentation explaining their purpose and when each action should be used. Consider adding doc comments following Go conventions.+// Action represents the result of an ingestion operation type Action int const ( + // ActionSkip indicates that the item should be skipped ActionSkip Action = iota + // ActionProcess indicates that the item should be processed ActionProcess )keychain-sdk/key_requests.go (1)
126-128: Add documentation and input validation.The method needs documentation and should validate its input parameters.
Consider these improvements:
+// keyRequests retrieves a batch of pending key requests for the specified keychain. +// It returns an error if the context is canceled or if the query fails. +// Parameters: +// - ctx: Context for the request +// - batchSize: Maximum number of requests to fetch (must be positive) +// - keychainId: ID of the keychain to fetch requests for func (a *AppClient) keyRequests(ctx context.Context, batchSize int, keychainId uint64) ([]*wardentypes.KeyRequest, error) { + if batchSize <= 0 { + return nil, fmt.Errorf("batch size must be positive, got %d", batchSize) + } return a.query.PendingKeyRequests(ctx, &client.PageRequest{Limit: uint64(batchSize)}, keychainId) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- keychain-sdk/config.go (2 hunks)
- keychain-sdk/internal/tracker/status_tracker.go (1 hunks)
- keychain-sdk/internal/tracker/tracker.go (1 hunks)
- keychain-sdk/key_requests.go (3 hunks)
- keychain-sdk/sign_requests.go (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- keychain-sdk/config.go
- keychain-sdk/sign_requests.go
🧰 Additional context used
📓 Path-based instructions (3)
keychain-sdk/internal/tracker/status_tracker.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/internal/tracker/tracker.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/key_requests.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (2)
keychain-sdk/internal/tracker/tracker.go (1)
15-17: Consider using uint16 for threshold.The
uint8type limits the threshold to a maximum value of 255. While this might be sufficient for current needs, usinguint16would provide more flexibility for future scaling without significant memory impact.keychain-sdk/internal/tracker/status_tracker.go (1)
23-36: LGTM! Well-implemented set operations.The stringSet methods are well-documented and efficiently implemented, providing clear success/failure indicators.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
keychain-sdk/internal/tracker/tracker.go (2)
7-12: Add documentation for the Action type and its constants.The new
Actiontype and its constants would benefit from documentation comments explaining their purpose and usage. This helps maintain code clarity and aids API users.Add documentation like this:
+// Action represents the result of ingesting a request type Action int const ( + // ActionSkip indicates the request should be skipped ActionSkip Action = iota + // ActionProcess indicates the request should be processed ActionProcess )
54-60: Add documentation for the Done method.The method would benefit from documentation explaining its purpose and when it should be called.
Add documentation like this:
+// Done marks the request with the given ID as completed and removes it from tracking func (t *T) Done(id uint64) { t.rw.Lock() defer t.rw.Unlock() delete(t.ingested, id) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- keychain-sdk/internal/tracker/status_tracker.go (1 hunks)
- keychain-sdk/internal/tracker/tracker.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- keychain-sdk/internal/tracker/status_tracker.go
🧰 Additional context used
📓 Path-based instructions (1)
keychain-sdk/internal/tracker/tracker.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Lint fixes Merge conflicts Added ClientsPool abstraction Build fixes Lint fix Lint fix Update keychain-sdk/tx_client_pool.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Code review fixes Merge branch 'feature/683-keychain-sdk-configure-multiple-nodes-endpoints' of github.com:warden-protocol/wardenprotocol into feature/683-keychain-sdk-configure-multiple-nodes-endpoints Added removal of processed requests Lint fix
backsapc
force-pushed
the
feature/683-keychain-sdk-configure-multiple-nodes-endpoints
branch
from
4dadd65 to
cd821e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (8)
keychain-sdk/config.go (1)
57-58: Enhance ConsensusNodeThreshold documentation and validation.The
ConsensusNodeThresholdfield would benefit from additional documentation specifying:
- Valid range of values (minimum and maximum thresholds)
- Behavior when threshold exceeds the number of available nodes
- Rationale for using uint8 type (max 255 nodes) and whether this might limit future scaling
keychain-sdk/keychain.go (2)
67-81: Enhance error context in clientsPool initializationConsider adding more context to the error by including the number of endpoints being initialized:
- return fmt.Errorf("failed to init connections: %w", err) + return fmt.Errorf("failed to initialize %d gRPC endpoint(s): %w", + len(a.config.GRPCConfigs), err)
117-118: Add documentation for ConnectionState return valueThe method should include documentation explaining the format of the returned map, where keys are endpoint URLs and values are their respective connection states.
// ConnectionState returns the current state of the gRPC connection. +// Returns a map where keys are gRPC endpoint URLs and values are their respective +// connectivity states. func (a *App) ConnectionState() map[string]connectivity.State {cmd/wardenkms/wardenkms.go (1)
180-194: Enhance input validation and error messagesConsider improving the function with:
- More descriptive error message
- Validation of individual node configs
Apply this diff:
func mapGrpcConfig(value GrpcNodeConfigDecoder) ([]keychain.GrpcNodeConfig, error) { if len(value) == 0 { - return nil, fmt.Errorf("GRPCUrls must be specified") + return nil, fmt.Errorf("at least one GRPC node configuration must be specified") } result := make([]keychain.GrpcNodeConfig, 0) for _, item := range value { + if item.GRPCUrl == "" { + return nil, fmt.Errorf("GRPC URL cannot be empty") + } result = append(result, keychain.GrpcNodeConfig{ GRPCInsecure: item.GRPCInsecure, GRPCURL: item.GRPCUrl, }) } return result, nil }keychain-sdk/internal/writer/writer.go (4)
Line range hint
59-64: Simplify context creation to avoid redundant wrapping.The initial call to
context.WithCancelis overwritten ifw.TxTimeout > 0, potentially causing unnecessary resource allocation. Consider restructuring the context creation to avoid redundant wrapping.Apply this diff to streamline the context handling:
func (w *W) Start(ctx context.Context, client SyncTxClient, flushErrors chan error) error { w.Logger.Info("starting tx writer") for { select { case <-ctx.Done(): return nil default: - ctx, cancel := context.WithCancel(ctx) + var cancel context.CancelFunc if w.TxTimeout > 0 { ctx, cancel = context.WithTimeout(ctx, w.TxTimeout) + } else { + ctx, cancel = context.WithCancel(ctx) } if err := w.Flush(ctx, client); err != nil { flushErrors <- err } cancel() time.Sleep(w.BatchInterval) } } }
120-120: Handle errors inFlushmethod consistently.Ensure that errors returned by
sendWaitTxare properly logged or wrapped to provide context for easier debugging.Consider modifying the error handling:
if err := w.sendWaitTx(ctx, txClient, msgers...); err != nil { for _, item := range msgs { item.Done <- err } + w.Logger.Error("failed to send transaction", "error", err) return err }
136-136: Enhance error context inBuildTxfailure.When
BuildTxfails, wrapping the error with additional context can aid in debugging.Modify the error return to include context:
if err != nil { - return err + return fmt.Errorf("failed to build transaction: %w", err) }Ensure you import the
"fmt"package if not already included.
141-141: Provide error context inSendWaitTxfailure.Adding context to errors from
SendWaitTxenhances clarity during troubleshooting.Adjust the error handling as follows:
if err = txClient.SendWaitTx(ctx, tx); err != nil { - return err + return fmt.Errorf("failed to send and wait for transaction: %w", err) }Again, ensure the
"fmt"package is imported.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- cmd/wardenkms/wardenkms.go (6 hunks)
- keychain-sdk/config.go (2 hunks)
- keychain-sdk/internal/tracker/status_tracker.go (1 hunks)
- keychain-sdk/internal/tracker/tracker.go (1 hunks)
- keychain-sdk/internal/writer/writer.go (5 hunks)
- keychain-sdk/key_requests.go (3 hunks)
- keychain-sdk/keychain.go (3 hunks)
- keychain-sdk/sign_requests.go (3 hunks)
- keychain-sdk/tx_client_pool.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- keychain-sdk/internal/tracker/tracker.go
- keychain-sdk/sign_requests.go
- keychain-sdk/tx_client_pool.go
🧰 Additional context used
📓 Path-based instructions (6)
cmd/wardenkms/wardenkms.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/internal/tracker/status_tracker.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/internal/writer/writer.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/key_requests.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/keychain.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (11)
keychain-sdk/config.go (2)
Line range hint
10-52: LGTM! Well-structured and documented configuration.The
BasicConfigstructure is well-organized with clear field documentation and appropriate types for each configuration parameter.
60-62: Verify node failover behavior.The configuration structure supports multiple nodes, but we should verify the implementation's behavior in various scenarios:
- Node failover when primary node is unavailable
- Load distribution across nodes
- Handling of node response conflicts
Let's check the implementation:
✅ Verification successful
Let me gather more information about the node failover and consensus handling implementation.
Let me search for more specific implementation details about the request tracking and consensus mechanism.
Let me try searching with a different approach to find the request processing and consensus implementation.
Let me check the tracker implementation since we found its location.
Based on the gathered information, I can now provide a final response about the node failover behavior.
Node failover and consensus implementation is properly handled
The implementation includes robust failover and consensus mechanisms:
- Uses a threshold-based tracker system that ensures requests are processed only after reaching consensus (ConsensusNodeThreshold)
- Maintains connection pool for multiple nodes via ClientsPool
- Implements request deduplication and tracking through the tracker package
- Handles request processing across nodes with proper status management
Key implementation details:
- Each request is tracked across nodes using a unique ID
- Requests are only processed once they meet the consensus threshold
- Failed nodes don't block the system as long as enough nodes are available to meet the threshold
- Request status is properly cleaned up after processing (tracker.Done)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for node failover and consensus handling logic rg -A 10 "GRPCConfigs" --type goLength of output: 2598
Script:
#!/bin/bash # Search for consensus and failover related code rg -A 10 "ConsensusNodeThreshold" --type go # Search for client pool related methods ast-grep --pattern 'func ($_ *TxClientPool) $_($$$) $_ { $$$ }'Length of output: 3392
Script:
#!/bin/bash # Search for tracker implementation rg -A 15 "func New\(" tracker/ rg -A 15 "func \(t \*Tracker\)" tracker/ # Search for how requests are processed across multiple nodes ast-grep --pattern 'func ($_ *App) process$_Request($$$) { $$$ }'Length of output: 243
Script:
#!/bin/bash # Search for request processing and consensus implementation rg -A 15 "keyRequestTracker|signRequestTracker" --type go # Search for any request processing methods ast-grep --pattern 'func ($_ *App) $_Request($$$) { $$$ }' # Look for tracker package fd trackerLength of output: 5302
Script:
#!/bin/bash # Check tracker implementation cat keychain-sdk/internal/tracker/tracker.go # Look for request processing in key_requests.go rg -A 20 "func \(a \*App\) processKeyRequest" keychain-sdk/key_requests.go # Look for client pool implementation rg -A 15 "type ClientsPool struct" keychain-sdk/Length of output: 1928
keychain-sdk/keychain.go (2)
32-33: LGTM: Clean addition of clientsPool fieldThe addition of
clientsPoolfield to the App struct is well-structured and aligns with the multi-node support objective.
84-92: LGTM with previous review considerationsThe implementation looks correct, taking into account the goroutine management suggestions from previous reviews.
cmd/wardenkms/wardenkms.go (2)
80-99: LGTM! Clean initialization with proper error handlingThe initialization of gRPC configs and app configuration follows Go best practices with proper error handling and structured configuration.
52-61: 🛠️ Refactor suggestionConsider using value receiver for Decode method
The
Decodemethod modifies the slice through a pointer receiver, but since slices already contain a pointer to the backing array, a value receiver would be more idiomatic in Go.Apply this diff:
-func (sd *GrpcNodeConfigDecoder) Decode(value string) error { +func (sd GrpcNodeConfigDecoder) Decode(value string) error { nodeConfigs := make([]GrpcNodeConfig, 0) if err := json.Unmarshal([]byte(value), &nodeConfigs); err != nil { return fmt.Errorf("invalid map json: %w", err) } - *sd = nodeConfigs + sd = nodeConfigs return nil }Likely invalid or redundant comment.
keychain-sdk/internal/writer/writer.go (5)
35-38: InterfaceSyncTxClientis well-defined and aligns with Go conventions.The
SyncTxClientinterface clearly specifies the required methods, promoting flexibility and decoupling in the codebase.
54-54: PassingSyncTxClienttoStartimproves modularity.Updating the
Startmethod to acceptclient SyncTxClientenhances flexibility by allowing different client implementations at runtime.
66-66: Ensure thread safety ofSyncTxClientimplementations.Since
clientis passed toFlushwithin a loop, verify that theSyncTxClientimplementation used is safe for concurrent operations or appropriately synchronized.
102-102:Flushmethod signature change enhances decoupling.Passing
txClient SyncTxClientas a parameter toFlushallows for greater flexibility and testing ease.
130-130: UpdatingsendWaitTxto accepttxClientimproves flexibility.This change allows different implementations of
SyncTxClientto be used, enhancing the modularity of the code.
…ints' of github.com:warden-protocol/wardenprotocol into feature/683-keychain-sdk-configure-multiple-nodes-endpoints
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (8)
cmd/wardenkms/health_check_response.go (1)
1-1: Add package documentation.According to Golang conventions, please add a package comment that describes the purpose of this package and its role in the health check system.
+// Package main implements the Warden KMS server and its health check functionality. package mainkeychain-sdk/internal/tracker/tracker.go (3)
15-24: Add package documentation for stringSet type.While the implementation is correct, consider adding more comprehensive documentation:
+// stringSet implements a thread-safe set of strings using a map type stringSet map[string]struct{} -// add safely adds a string to the set +// add safely adds a string to the set and returns false if the string already exists func (s stringSet) add(str string) bool {
32-36: Consider using uint16 for threshold.The current
uint8type limits the threshold to a maximum of 255. For a system designed to handle multiple node endpoints, this might be too restrictive in the future.- threshold uint8 + threshold uint16 -func New(threshold uint8) *T { +func New(threshold uint16) *T {
39-47: Add documentation for ingestTracker method.Consider adding documentation to clarify the method's purpose and locking requirements:
+// ingestTracker returns or initializes a stringSet for the given ID. +// Note: Caller must hold the lock. func (t *T) ingestTracker(id uint64) stringSet {cmd/wardenkms/wardenkms.go (1)
152-186: Consider pre-allocating nodes slice with proper capacityThe implementation is correct, but we can optimize memory allocation.
Apply this diff:
- nodes := make([]NodeStatus, 0, len(connectionStates)) + nodes := make([]NodeStatus, len(connectionStates)) + i := 0 for url, state := range connectionStates { if state == connectivity.Ready { readyConnectionsCount += 1 } - nodes = append(nodes, NodeStatus{ + nodes[i] = NodeStatus{ Address: url, Status: state.String(), }) + i++ }keychain-sdk/tx_client_pool.go (3)
25-25: Add documentation comment for exported functionNewClientsPool.The function
NewClientsPoolis exported but lacks a documentation comment. As per the Uber Go Style Guide, all exported functions should have a comment explaining their purpose.Consider adding a comment like this:
// NewClientsPool creates a new ClientsPool with the provided configuration. func NewClientsPool(config Config) *ClientsPool { // ... }
13-18: Add struct comments for exported typesAppClientandClientsPool.The types
AppClientandClientsPoolare exported but do not have documentation comments. According to the Uber Go Style Guide, exported types should include comments that describe their purpose.Consider adding comments like the following:
// AppClient represents a client connection to a single gRPC node. type AppClient struct { grpcURL string grpcInsecure bool query *client.QueryClient txClient *client.TxClient } // ClientsPool manages multiple AppClient instances for handling gRPC connections. type ClientsPool struct { clients []*AppClient config Config }This provides clarity for other developers who may use these types.
62-62: Ensure consistent logging keys and values.In the logging statement, the keys
"url"and"insecure"are associated with their respective values. Verify that this aligns with your logging standards and consider adding context if necessary.No code change is required if the current implementation meets your logging format.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- cmd/wardenkms/health_check_response.go (1 hunks)
- cmd/wardenkms/wardenkms.go (6 hunks)
- docker-compose.yml (1 hunks)
- keychain-sdk/config.go (2 hunks)
- keychain-sdk/internal/tracker/tracker.go (1 hunks)
- keychain-sdk/tx_client_pool.go (1 hunks)
- warden/x/act/client/actions.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- keychain-sdk/config.go
🧰 Additional context used
📓 Path-based instructions (5)
cmd/wardenkms/health_check_response.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.cmd/wardenkms/wardenkms.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/internal/tracker/tracker.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.keychain-sdk/tx_client_pool.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.warden/x/act/client/actions.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (2)
cmd/wardenkms/wardenkms.go (1)
Learnt from: backsapc PR: warden-protocol/wardenprotocol#810 File: cmd/wardenkms/wardenkms.go:45-50 Timestamp: 2024-10-24T13:01:55.558Z Learning: In the `cmd/wardenkms/wardenkms.go` file and the codebase, use `Grpc` instead of `GRPC` for acronym casing in type names, such as `GrpcNodeConfig` and `GrpcNodeConfigDecoder`, even though Go naming conventions suggest capitalizing acronyms.keychain-sdk/internal/tracker/tracker.go (1)
Learnt from: backsapc PR: warden-protocol/wardenprotocol#810 File: keychain-sdk/internal/tracker/tracker.go:0-0 Timestamp: 2024-10-24T13:35:40.452Z Learning: In `keychain-sdk/internal/tracker/tracker.go`, the `statusTracker` method is always called with the lock already held by the caller, so it does not need to acquire a lock itself.
🔇 Additional comments (8)
keychain-sdk/internal/tracker/tracker.go (2)
8-13: LGTM! Clear and idiomatic type definitions.The
Actiontype and its constants are well-defined using idiomatic Go patterns withiota.
66-72: LGTM! Clean and thread-safe implementation.The
Donemethod correctly handles cleanup with proper locking.docker-compose.yml (1)
18-18: Verify the gas price denomination change from 'uward' to 'award'The change from
"0uward"(micro-ward) to"0award"(atto-ward) represents a significant shift in denomination magnitude. This could impact transaction costs and validation across nodes.Please ensure:
- This denomination change is intentional
- All nodes in the network use consistent denomination
✅ Verification successful
Based on the search results, I can now generate the final response as there is clear evidence about the denomination usage across the codebase:
Denomination change from 'uward' to 'award' is correct and intentional
The change from
uwardtoawardin the minimum gas prices is consistent with the broader codebase:
- Local development and testing environments use
awarddenomination- Production configurations in Chiado testnet use
award(e.g.,250000000000000award)- Only the legacy Buenavista testnet uses
uward- Documentation and code comments confirm
awardis the atto-ward denomination (0.000000000000000001 WARD)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other gas price configurations in the codebase # to verify consistency across different environments echo "Searching for gas price configurations:" rg -i "minimum.?gas.?prices?" echo -e "\nSearching for ward denomination usage:" rg -i "[ua]ward"Length of output: 25238
cmd/wardenkms/wardenkms.go (3)
98-99: Ensure type consistency for ConsensusNodeThresholdThere's a type mismatch between
Config.ConsensusNodeThreshold(uint8) and its usage. This could lead to potential issues.Let's verify the type in the keychain package:
45-48:⚠️ Potential issueFix inconsistent field naming in GrpcNodeConfig struct
The field name
GRPCUrlshould beGRPCURLfor consistency with the rest of the codebase.Apply this diff:
type GrpcNodeConfig struct { - GRPCUrl string + GRPCURL string GRPCInsecure bool }Likely invalid or redundant comment.
25-25:⚠️ Potential issueFix inconsistent field naming in default JSON config
The default JSON config uses
GRPCUrlwhile the struct field is namedGRPCURL. This inconsistency could cause confusion.Update the default JSON to match the struct field name:
-GRPCURLs GrpcNodeConfigDecoder `env:"GRPC_URLS, default=[{\"GRPCUrl\":\"localhost:9090\",\"GRPCInsecure\":true}] "` +GRPCURLs GrpcNodeConfigDecoder `env:"GRPC_URLS, default=[{\"GRPCURL\":\"localhost:9090\",\"GRPCInsecure\":true}] "`Likely invalid or redundant comment.
warden/x/act/client/actions.go (2)
Line range hint
1-283: LGTM! Code follows Go best practices.The file demonstrates good adherence to Go best practices and the Uber style guide:
- Clear error handling with descriptive messages
- Well-documented functions
- Consistent naming conventions
- Appropriate use of constants and types
144-144: Verify the token denomination change.The help text example for coin denomination has been changed from "10uward" to "10award". Please verify if "award" is the correct denomination as per the project's token standards.
✅ Verification successful
Token denomination change to "award" is correct
The verification confirms that "award" is the correct token denomination across the codebase:
- It's consistently used in test cases for transactions and balances
- Set as the default denomination in configuration files (cmd/faucet/pkg/config/config.go)
- Used as the default fee token in the go-client (DefaultFees)
- There's even a comment in cmd/faucet/main.go indicating tokens "will be set as WARD not uWARD"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for token denomination usage across the codebase echo "Searching for token denomination usage..." rg -i "award" --type go rg -i "uward" --type go # Search for denomination constants or configurations echo "Searching for denomination definitions..." ast-grep --pattern 'const $_ = "$_ward"'Length of output: 4047
warden/x/act/client/actions.go
Outdated
| @@ -141,7 +141,7 @@ func addFlagsFromMsg(msg sdk.Msg, cmd *cobra.Command) { | |||
| cmd.Flags().BytesBase64(flagName, nil, "") | |||
| case reflect.Struct: | |||
| if v.Field(i).Type().Elem().AssignableTo(reflect.TypeOf(sdk.Coin{})) { | |||
| cmd.Flags().StringSlice(flagName, nil, "(e.g. 10uward)") | |||
| cmd.Flags().StringSlice(flagName, nil, "(e.g. 10award)") | |||
There was a problem hiding this comment.
this change is unrelated to this PR, please open a separate PR to fix bugs
cmd/wardenkms/wardenkms.go
Outdated
| @@ -56,18 +77,26 @@ func main() { | |||
| return | |||
| } | |||
|
|
|||
| grpcConfigs, err := mapGrpcConfig(cfg.GRPCURLs) | |||
There was a problem hiding this comment.
since wardenkms is an user of the keychain-sdk, having to do this sort of mapping "manually" shows that the configuration of the keychain-sdk is not very ergonomic
i.e. keychain-sdk is not a library that I would enjoy using
i think we can explore different configuration patterns such as the option pattern maybe, or even treating the URL string as "special", like if it starts with "http://" or "tcp://" turn on the insecure option, while if it starts with "https://" turn insecure off
but, if you have better ideas please share them!
|
I don't know if I'm doing something wrong but I'm trying to run it with: and it doesn't seem to be working? I tried logging the cfg: and it's weird cause there are two "empty" elements (i.e. the url is not set) how should I run it? |
You did everything right; the problem was on my end. I followed your suggestion and simplified the wardenkms configuration. I also fixed the startup issue. |
keychain-sdk/config.go
Outdated
| @@ -7,23 +7,14 @@ import ( | |||
| sdk "github.com/cosmos/cosmos-sdk/types" | |||
| ) | |||
|
|
|||
| // Config is the configuration for the Keychain. | |||
| type Config struct { | |||
| type BasicConfig struct { | |||
There was a problem hiding this comment.
what's the rationale behind splitting Config into Config and BasicConfig? I think I might be missing something
keychain-sdk/config.go
Outdated
| type GrpcNodeConfig struct { | ||
| // GRPCInsecure determines whether to allow an insecure connection to the | ||
| // gRPC server. | ||
| GRPCInsecure bool | ||
|
|
||
| // GRPCURL is the URL of the gRPC server to connect to. | ||
| // e.g. "localhost:9090" | ||
| GRPCURL string | ||
| } |
There was a problem hiding this comment.
since we are inside a struct called "GrpcNodeConfig", let's avoid repeating the word GRPC everywhere:
| type GrpcNodeConfig struct { | |
| // GRPCInsecure determines whether to allow an insecure connection to the | |
| // gRPC server. | |
| GRPCInsecure bool | |
| // GRPCURL is the URL of the gRPC server to connect to. | |
| // e.g. "localhost:9090" | |
| GRPCURL string | |
| } | |
| type GrpcNodeConfig struct { | |
| // Insecure determines whether to allow an insecure connection to the | |
| // gRPC server. | |
| Insecure bool | |
| // Host is the address of the gRPC server to connect to. | |
| // e.g. "localhost:9090" | |
| Host string | |
| } |
technically, the second field is not a URL since it doesn't contain a protocol scheme, i renamed it into "host"
keychain-sdk/config.go
Outdated
| GRPCConfigs []GrpcNodeConfig | ||
| } | ||
|
|
||
| type GrpcNodeConfig struct { |
There was a problem hiding this comment.
the case of the word grpc is inconsistent
| type GrpcNodeConfig struct { | |
| type GRPCNodeConfig struct { |
docker-compose.yml
Outdated
| @@ -15,7 +15,7 @@ services: | |||
| WARDEND_GRPC_ADDRESS: 0.0.0.0:9090 | |||
| WARDEND_RPC_LADDR: tcp://0.0.0.0:26657 | |||
| WARDEND_RPC_CORS_ALLOWED_ORIGINS: "*" | |||
| WARDEND_MINIMUM_GAS_PRICES: "0uward" | |||
| WARDEND_MINIMUM_GAS_PRICES: "0award" | |||
There was a problem hiding this comment.
let's fix this in another PR since it's not really related to the keychain sdk
keychain-sdk/config.go
Outdated
| // ConsensusNodeThreshold represents the number of nodes required to execute a pending key/sign request. | ||
| ConsensusNodeThreshold uint8 | ||
|
|
||
| // GRPCURLs is the list of URLs of the gRPC server to connect to. | ||
| // e.g. "localhost:9090" | ||
| GRPCConfigs []GrpcNodeConfig |
There was a problem hiding this comment.
since we call them "nodes" (ConsensusNodeThreshold represents the number of nodes...) we should be consistent. Also, the doc comment for GRPCConfigs is outdated.
| // ConsensusNodeThreshold represents the number of nodes required to execute a pending key/sign request. | |
| ConsensusNodeThreshold uint8 | |
| // GRPCURLs is the list of URLs of the gRPC server to connect to. | |
| // e.g. "localhost:9090" | |
| GRPCConfigs []GrpcNodeConfig | |
| // ConsensusNodeThreshold represents the number of nodes required to execute a pending key/sign request. | |
| ConsensusNodeThreshold uint8 | |
| // Nodes is the list of gRPC nodes to connect to for fetching incoming requests and broadcasting transactions. | |
| Nodes []GrpcNodeConfig |
| type HealthCheckResponse struct { | ||
| // The number of nodes that are online | ||
| Online uint `json:"online"` | ||
|
|
||
| // The total number of nodes | ||
| Total uint `json:"total"` | ||
|
|
||
| // The consensus threshold | ||
| Threshold uint8 `json:"threshold"` | ||
|
|
||
| // Node statuses | ||
| Nodes []NodeStatus `json:"nodes"` | ||
| } | ||
|
|
||
| type NodeStatus struct { | ||
| // The address of the node | ||
| Address string `json:"address"` | ||
|
|
||
| // The status of the node | ||
| Status string `json:"status"` | ||
| } |
There was a problem hiding this comment.
Let's follow Go conventions when documenting types or fields: https://go.dev/wiki/CodeReviewComments#comment-sentences.
| type HealthCheckResponse struct { | |
| // The number of nodes that are online | |
| Online uint `json:"online"` | |
| // The total number of nodes | |
| Total uint `json:"total"` | |
| // The consensus threshold | |
| Threshold uint8 `json:"threshold"` | |
| // Node statuses | |
| Nodes []NodeStatus `json:"nodes"` | |
| } | |
| type NodeStatus struct { | |
| // The address of the node | |
| Address string `json:"address"` | |
| // The status of the node | |
| Status string `json:"status"` | |
| } | |
| type HealthCheckResponse struct { | |
| // Online is... | |
| Online uint `json:"online"` | |
| // Total is... | |
| Total uint `json:"total"` | |
| // Threshold is... | |
| Threshold uint8 `json:"threshold"` | |
| // Nodes... | |
| Nodes []NodeStatus `json:"nodes"` | |
| } | |
| type NodeStatus struct { | |
| // Address... | |
| Address string `json:"address"` | |
| // Status... | |
| Status string `json:"status"` | |
| } |
keychain-sdk/tx_client_pool.go
Outdated
| msgers ...client.Msger) ([]byte, error) { | ||
| liveClient, err := p.liveTxClient() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to aquire live client for BuildTx: %w", err) |
keychain-sdk/tx_client_pool.go
Outdated
| "google.golang.org/grpc/connectivity" | ||
| ) | ||
|
|
||
| type AppClient struct { |
There was a problem hiding this comment.
Let’s avoid using the term “app” which is overloaded (in this case we’re connecting to a warden node, it can be confused with the keychain app itself), plus avoid exporting it because as far as I understand the use won’t initialize this manually
| type AppClient struct { | |
| type wardenClient struct { |
Co-authored-by: Antonio Pitasi <[email protected]>
Co-authored-by: Antonio Pitasi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
keychain-sdk/example_keychain_test.go (1)
Line range hint
15-15: Follow Go's example testing conventions.This example should follow Go's conventions for example tests:
- Use the
Exampleprefix in the function name- Include expected output in comments
Apply this diff to follow the conventions:
-func Main() { +func ExampleNewApp() { // ... existing code ... + + // Output:
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
keychain-sdk/example_keychain_test.go(1 hunks)keychain-sdk/tx_client_pool.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- keychain-sdk/tx_client_pool.go
🧰 Additional context used
📓 Path-based instructions (1)
keychain-sdk/example_keychain_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (1)
keychain-sdk/example_keychain_test.go (1)
37-38: Reference existing feedback on multi-node configuration.
Please refer to the previous review comment about enhancing the example to demonstrate multi-node configuration.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
keychain-sdk/tx_client_pool.go (3)
13-23: Add documentation for exported types and their fields.While the type names are descriptive, adding documentation comments would improve code maintainability and help users understand the purpose of each type and its fields.
Add documentation like this:
+// wardenClient represents a single gRPC client connection to a Warden Protocol node. type wardenClient struct { grpcURL string grpcInsecure bool query *client.QueryClient txClient *client.TxClient } +// clientsPool manages a collection of wardenClient instances and their configuration. type clientsPool struct { clients []*wardenClient config Config }
93-113: Add context timeout handling for transaction operations.While the methods accept a context parameter, they don't explicitly handle timeouts or cancellation signals.
Consider adding timeout handling:
func (cp *clientsPool) BuildTx(ctx context.Context, gasLimit uint64, fees sdkTypes.Coins, msgers ...client.Msger) ([]byte, error) { + if ctx.Err() != nil { + return nil, fmt.Errorf("context error before building tx: %w", ctx.Err()) + } liveClient, err := cp.liveTxClient() if err != nil { return nil, fmt.Errorf("failed to acquire live client for BuildTx: %w", err) } return liveClient.BuildTx(ctx, gasLimit, fees, msgers...) }
115-124: Consider caching connection states.Frequent calls to
ConnectionState()could impact performance as it checks the state of every connection on each call.Consider implementing state caching with a configurable refresh interval:
type clientsPool struct { clients []*wardenClient config Config + stateCache map[string]connectivity.State + lastStateCheck time.Time + stateTTL time.Duration } func (cp *clientsPool) ConnectionState() map[string]connectivity.State { + if time.Since(cp.lastStateCheck) < cp.stateTTL { + return cp.stateCache + } statuses := make(map[string]connectivity.State) for _, appClient := range cp.clients { state := appClient.query.Conn().GetState() statuses[appClient.grpcURL] = state } + cp.stateCache = statuses + cp.lastStateCheck = time.Now() return statuses }keychain-sdk/internal/writer/writer.go (4)
35-38: LGTM! Consider adding interface documentation.The interface is well-defined with clear method signatures. Consider adding GoDoc comments to document the interface and its methods.
Add documentation like this:
+// SyncTxClient defines the interface for synchronous transaction operations type SyncTxClient interface { + // SendWaitTx sends a transaction and waits for its inclusion in a block SendWaitTx(ctx context.Context, txBytes []byte) (string, error) + // BuildTx constructs a transaction from the provided messages BuildTx(ctx context.Context, gasLimit uint64, fees sdk.Coins, msgers ...client.Msger) ([]byte, error) }
Line range hint
54-69: LGTM! Consider enhancing error logging.The method correctly handles context cancellation and timeout. The client parameter is properly passed to Flush.
Consider logging the error before sending it to the channel:
- if err := w.Flush(ctx, client); err != nil { - flushErrors <- err - } + if err := w.Flush(ctx, client); err != nil { + w.Logger.Error("flush failed", "error", err) + flushErrors <- err + }
Line range hint
102-127: LGTM! Consider adding batch size logging.The method handles batch processing and cleanup correctly. The error handling is comprehensive.
Consider logging the batch size when starting the flush:
func (w *W) Flush(ctx context.Context, txClient SyncTxClient) error { msgs := w.batch.Clear() + w.Logger.Debug("starting flush", "batch_size", len(msgs)) if len(msgs) == 0 { w.Logger.Debug("flushing batch", "empty", true) return nil }
Line range hint
130-148: LGTM! Consider adding transaction timing metrics.The method correctly handles transaction building and sending with proper synchronization.
Consider adding timing metrics to track transaction performance:
func (w *W) sendWaitTx(ctx context.Context, txClient SyncTxClient, msgs ...client.Msger) error { w.sendTxLock.Lock() defer w.sendTxLock.Unlock() + start := time.Now() w.Logger.Info("flushing batch", "count", len(msgs)) tx, err := txClient.BuildTx(ctx, w.gasLimit(), w.fees(), msgs...) if err != nil { return err } hash, err := txClient.SendWaitTx(ctx, tx) if err != nil { return err } - w.Logger.Info("flush complete", "tx_hash", hash) + w.Logger.Info("flush complete", "tx_hash", hash, "duration", time.Since(start)) return nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
cmd/wardenkms/health_check_response.go(1 hunks)cmd/wardenkms/wardenkms.go(5 hunks)keychain-sdk/config.go(1 hunks)keychain-sdk/example_keychain_test.go(1 hunks)keychain-sdk/internal/writer/writer.go(5 hunks)keychain-sdk/key_requests.go(3 hunks)keychain-sdk/keychain.go(3 hunks)keychain-sdk/sign_requests.go(3 hunks)keychain-sdk/tx_client_pool.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/wardenkms/health_check_response.go
- cmd/wardenkms/wardenkms.go
- keychain-sdk/example_keychain_test.go
🧰 Additional context used
📓 Path-based instructions (6)
keychain-sdk/config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
keychain-sdk/internal/writer/writer.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
keychain-sdk/key_requests.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
keychain-sdk/keychain.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
keychain-sdk/sign_requests.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
keychain-sdk/tx_client_pool.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (2)
keychain-sdk/config.go (1)
Learnt from: backsapc
PR: warden-protocol/wardenprotocol#810
File: cmd/wardenkms/wardenkms.go:45-50
Timestamp: 2024-11-12T08:05:41.584Z
Learning: In the `cmd/wardenkms/wardenkms.go` file and the codebase, use `Grpc` instead of `GRPC` for acronym casing in type names, such as `GrpcNodeConfig` and `GrpcNodeConfigDecoder`, even though Go naming conventions suggest capitalizing acronyms.
keychain-sdk/keychain.go (1)
Learnt from: backsapc
PR: warden-protocol/wardenprotocol#810
File: keychain-sdk/keychain.go:74-81
Timestamp: 2024-11-12T08:05:41.584Z
Learning: The team prefers not to refactor the `writer.New` constructor to use functional options in `keychain-sdk/keychain.go`.
🔇 Additional comments (16)
keychain-sdk/config.go (2)
56-57: Update documentation to match field naming
The comment refers to "URLs" but the GRPCNodeConfig uses "Host". Consider updating for consistency:
- // Nodes is the list of URLs of the gRPC server to connect to.
+ // Nodes is the list of gRPC nodes to connect to.60-67: 🛠️ Refactor suggestion
Multiple improvements needed for GRPCNodeConfig
- Based on project conventions, rename the struct to use consistent casing:
-type GRPCNodeConfig struct {
+type GrpcNodeConfig struct {- Update Host field documentation:
- // Host is the URL of the gRPC server to connect to.
+ // Host is the address of the gRPC server to connect to.- Consider adding validation for the Host field format.
keychain-sdk/keychain.go (4)
32-33: LGTM: Addition of clientsPool field
The addition of clientsPool field aligns well with the PR objective of supporting multiple node endpoints.
40-41: LGTM: Tracker initialization with consensus threshold
The initialization of trackers with ConsensusNodeThreshold is properly implemented.
117-118: LGTM: Updated ConnectionState to support multiple clients
The method has been properly updated to return a map of connection states, which aligns with the multi-client architecture.
67-97: Verify error handling consistency across the codebase
The implementation of client pool initialization and goroutine management looks good. Let's verify the error handling pattern is consistent.
✅ Verification successful
Error handling implementation is consistent and appropriate
The error handling patterns across the codebase show a well-structured approach:
- The main goroutine properly propagates errors through the
flushErrorschannel - Ingestion methods (
ingestKeyRequestsandingestSignRequests) appropriately log errors and continue processing - The writer implementation includes proper context handling and error propagation
- All error cases are properly logged with descriptive messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling patterns in similar goroutines
rg -A 5 'go func\(\).*{.*if err :=.*; err != nil.*{.*logger.*Error'
Length of output: 223
Script:
#!/bin/bash
# Search for error handling patterns in goroutines
rg -A 5 "go func\(\)"
# Also check error handling in the ingestion methods
ast-grep --pattern 'func $_(signRequestsCh chan *wardentypes.SignRequest, $_) {
$$$
}'
ast-grep --pattern 'func $_(keyRequestsCh chan *wardentypes.KeyRequest, $_) {
$$$
}'
Length of output: 4818
Script:
#!/bin/bash
# Check the implementation of ingestion methods
rg -A 10 "func.*ingestKeyRequests"
rg -A 10 "func.*ingestSignRequests"
# Check error handling in the writer
rg -A 10 "func.*Start.*context.Context.*error"
Length of output: 3135
keychain-sdk/tx_client_pool.go (3)
1-11: LGTM! Package structure and imports are well-organized.
The imports are properly organized and all are being used in the code.
34-77: 🛠️ Refactor suggestion
Improve error handling in connection initialization.
The current implementation stops at the first connection failure. For better resilience, continue initializing remaining connections even if some fail.
This is a duplicate of a previous review comment suggesting to modify the connection initialization to handle partial failures gracefully.
79-91: 🛠️ Refactor suggestion
Implement load balancing for client selection.
The current implementation always selects the first available client, which could lead to uneven load distribution.
This is a duplicate of a previous review comment suggesting to implement load balancing strategies.
keychain-sdk/key_requests.go (2)
126-128: LGTM! Clean and focused implementation.
The function has been correctly moved to the wardenClient type and properly handles the new parameters.
77-95: 🛠️ Refactor suggestion
Improve channel operation safety and add context support.
The channel send operation could block indefinitely and there's no way to cancel the operation.
func (a *App) ingestRequest(
+ ctx context.Context,
keyRequestsCh chan *wardentypes.KeyRequest,
keyRequest *wardentypes.KeyRequest,
client *wardenClient) {
action, err := a.keyRequestTracker.Ingest(keyRequest.Id, client.grpcURL)
if err != nil {
a.logger().Error("failed to ingest key request", "id", keyRequest.Id, "grpcUrl", client.grpcURL, "error", err)
return
}
if action == tracker.ActionSkip {
a.logger().Debug("skipping key request", "id", keyRequest.Id, "grpcUrl", client.grpcURL)
return
}
if action == tracker.ActionProcess {
- keyRequestsCh <- keyRequest
+ select {
+ case keyRequestsCh <- keyRequest:
+ a.logger().Debug("request queued for processing", "id", keyRequest.Id)
+ case <-ctx.Done():
+ a.logger().Debug("context cancelled while queueing request", "id", keyRequest.Id)
+ default:
+ a.logger().Warn("processing queue full", "id", keyRequest.Id)
+ }
}
}Likely invalid or redundant comment.
keychain-sdk/sign_requests.go (5)
11-11: LGTM: Import addition aligns with new tracker functionality
The addition of the tracker import supports the new request tracking capabilities.
Line range hint 16-24: LGTM: Interface naming improved
The renaming from SignaResponseWriter to SignResponseWriter improves naming consistency and fixes the typo.
72-86: Consider making the timeout configurable
The hardcoded 5-second timeout might not be suitable for all network conditions. Consider making it configurable through the app configuration.
+// DefaultSignRequestTimeout defines the default timeout for sign request operations
+const DefaultSignRequestTimeout = 5 * time.Second
func (a *App) ingestSignRequests(signRequestsCh chan *wardentypes.SignRequest, appClient *wardenClient) {
for {
- reqCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ timeout := a.config.SignRequestTimeout
+ if timeout == 0 {
+ timeout = DefaultSignRequestTimeout
+ }
+ reqCtx, cancel := context.WithTimeout(context.Background(), timeout)89-104: LGTM: Well-structured request ingestion logic
The new method effectively handles individual sign requests with:
- Proper error handling and logging
- Clear control flow using the tracker
- Good separation of concerns
143-145: Verify the integration of the moved signRequests method
The method has been moved to wardenClient. Let's verify that all callers have been updated accordingly.
✅ Verification successful
Integration of moved signRequests method is complete
The method has been properly moved to wardenClient and all references have been updated correctly. The only remaining references to App.signRequests are in the context of the method being called through the wardenClient instance, showing proper integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old App.signRequests method
rg -l "App.*signRequests" --type go
# Search for all current uses of wardenClient.signRequests
rg -l "wardenClient.*signRequests" --type go
Length of output: 174
Script:
#!/bin/bash
# Check the actual usage context in IWarden.go
rg "App.*signRequests" precompiles/warden/IWarden.go -A 5 -B 5
# Check the actual usage context in sign_requests.go
rg "App.*signRequests" keychain-sdk/sign_requests.go -A 5 -B 5
Length of output: 37007
#683
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation