#683 keychain sdk configure multiple nodes endpoints #810
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request refactors configuration and client management by replacing single gRPC connection fields with multiple node configurations and a consensus threshold. It introduces a clients pool for managing multiple gRPC clients, updates key and sign request ingestion to handle multiple clients, enhances health check responses with node statuses, and refactors internal tracking and writer components for improved modularity. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
Pitasi
left a comment
Pitasi
left a comment
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
| } | ||
| } | ||
|
|
||
| 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
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
|
|
||
| // 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
| 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
| "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 <antonio@pitasi.dev>
Co-authored-by: Antonio Pitasi <antonio@pitasi.dev>
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
…sdk-configure-multiple-nodes-endpoints
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
keychain-sdk/sign_requests.go (1)
72-87: Goroutine loop ignores cancellation context
The infinitefor { … time.Sleep … }loop never checksctx.Done(). WhenApp.Startreturns, these goroutines keep running and may leak resources.A previous review already raised this for the key-request ingest path; the same fix is needed here (select on
ctx.Done()ortime.After).
🧹 Nitpick comments (2)
keychain-sdk/sign_requests.go (1)
51-57:onCompleteis executed even when the write fails
Callingw.onComplete()unconditionally aftertxWriter.Writewill mark the request as done even when the write fails, potentially losing the request forever. Execute it only on success (or introduce a retry / back-off policy).-err := w.txWriter.Write(ctx, client.SignRequestFulfilment{ … }) -w.onComplete() +wErr := w.txWriter.Write(ctx, client.SignRequestFulfilment{ … }) +if wErr == nil { + w.onComplete() +} -w.logger.Debug("fulfilled sign request", "id", w.signRequestID, "error", err) - -return err +w.logger.Debug("fulfilled sign request", "id", w.signRequestID, "error", wErr) + +return wErrkeychain-sdk/keychain.go (1)
117-120:ConnectionStatepanics ifStarthas not been called
a.clientsPoolis assigned inStart. If a caller invokesConnectionStatebeforehand (e.g., for a readiness probe), a nil-pointer dereference occurs.if a.clientsPool == nil { return nil } return a.clientsPool.ConnectionState()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/wardenkms/wardenkms.go(5 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)
🚧 Files skipped from review as they are similar to previous changes (4)
- keychain-sdk/example_keychain_test.go
- cmd/wardenkms/wardenkms.go
- keychain-sdk/key_requests.go
- keychain-sdk/internal/writer/writer.go
🧰 Additional context used
🧠 Learnings (1)
keychain-sdk/keychain.go (2)
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.
Learnt from: backsapc
PR: warden-protocol/wardenprotocol#810
File: keychain-sdk/keychain.go:74-81
Timestamp: 2024-10-24T13:03:23.718Z
Learning: The team prefers not to refactor the `writer.New` constructor to use functional options in `keychain-sdk/keychain.go`.
🧬 Code Graph Analysis (1)
keychain-sdk/keychain.go (4)
cmd/wardenkms/wardenkms.go (1)
Config(23-44)keychain-sdk/config.go (1)
Config(11-58)keychain-sdk/internal/writer/writer.go (1)
New(41-53)keychain-sdk/internal/tracker/tracker.go (1)
New(32-37)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: lint
- GitHub Check: lint
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
keychain-sdk/internal/tracker/tracker.go (1)
32-37: 🛠️ Refactor suggestionConsider adding threshold validation
The constructor doesn't validate that the threshold is greater than zero, which could lead to incorrect behavior if a zero value is provided.
func New(threshold uint8) *T { + if threshold == 0 { + panic("threshold must be greater than zero") + } return &T{ threshold: threshold, ingested: make(map[uint64]stringSet), } }
🧹 Nitpick comments (3)
keychain-sdk/internal/tracker/tracker.go (3)
27-30: Consider adding struct documentationThe
Tstruct lacks documentation explaining its purpose. Adding a comment would help new developers understand this component's role in the consensus tracking system.+// T implements a thread-safe tracker for consensus-based ingestion processing type T struct { threshold uint8 rw sync.RWMutex ingested map[uint64]stringSet }
39-48: Consider renaming ingestTracker method to follow Go conventionsMethod names in Go are typically verbs. Consider renaming
ingestTrackertogetOrCreateIngestTrackerto better reflect its action.-func (t *T) ingestTracker(id uint64) stringSet { +func (t *T) getOrCreateIngestTracker(id uint64) stringSet { value, ok := t.ingested[id] if !ok { t.ingested[id] = make(stringSet) return t.ingested[id] } return value }
50-65: Simplify type conversion in threshold comparisonThe type conversions in the threshold comparison are unnecessary and could be simplified.
- if uint64(len(value)) < uint64(t.threshold) { + if len(value) < int(t.threshold) { return ActionSkip, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
keychain-sdk/internal/tracker/tracker.go(1 hunks)keychain-sdk/sign_requests.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- keychain-sdk/sign_requests.go
🧰 Additional context used
🧠 Learnings (1)
keychain-sdk/internal/tracker/tracker.go (3)
Learnt from: backsapc
PR: warden-protocol/wardenprotocol#810
File: keychain-sdk/internal/tracker/tracker.go:20-20
Timestamp: 2024-10-23T10:26:05.989Z
Learning: In the `keychain-sdk/internal/tracker/tracker.go` file, the variable `nodeUrl` should remain as `nodeUrl` and not be renamed to `nodeURL`.
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.
Learnt from: backsapc
PR: warden-protocol/wardenprotocol#810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The `MarkProcessed` method has been removed from `keychain-sdk/internal/tracker/status_tracker.go` in the Go codebase, as it is no longer needed.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint
- GitHub Check: Analyze (go)
- GitHub Check: lint
🔇 Additional comments (3)
keychain-sdk/internal/tracker/tracker.go (3)
8-13: Good addition of Action enum for clear return valuesThis addition of the
Actiontype with clear constants improves code readability and type safety. Usingiotafor incremental constants follows Go idioms and keeps the implementation clean.
15-24: Good implementation of stringSet with safe add methodRenaming from
hashSettostringSetbetter reflects the purpose of this data structure. Theaddmethod safely adds elements while preventing duplicates and returns a boolean indicating success, which is a good design pattern.
67-72: LGTM: Clean implementation of the Done methodThe
Donemethod is properly implemented with appropriate locking to safely remove tracked IDs.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
keychain-sdk/tx_client_pool.go (3)
111-111: Great job fixing the typo in error messages.The error message correctly uses "acquire" instead of "aquire", addressing the issue mentioned in previous reviews.
26-33: 🛠️ Refactor suggestionAdd configuration validation in newClientsPool.
The constructor should validate that the configuration has at least one node configured before creating the pool. This would prevent potential issues later when trying to initialize or use an empty pool.
func newClientsPool(config Config) *clientsPool { + if len(config.Nodes) == 0 { + panic("at least one node configuration is required") + } pool := clientsPool{ clients: make([]*wardenClient, 0), config: config, } return &pool }
35-51: 🛠️ Refactor suggestionContinue initializing connections even if some fail.
Currently, if any connection initialization fails, the method returns immediately without trying to connect to other nodes. In a distributed system, it's better to attempt connecting to all configured nodes and only return an error if none succeed.
func (cp *clientsPool) initConnections(logger *slog.Logger) error { identity, err := client.NewIdentityFromSeed(cp.config.Mnemonic) if err != nil { return fmt.Errorf("failed to create identity: %w", err) } + var initErrors []error for _, grpcURL := range cp.config.Nodes { appClient, err := cp.initConnection(logger, grpcURL, cp.config.ChainID, identity) if err != nil { - return err + logger.Error("failed to initialize connection", "url", grpcURL.Host, "error", err) + initErrors = append(initErrors, err) + continue } cp.clients = append(cp.clients, appClient) } + if len(cp.clients) == 0 { + return fmt.Errorf("failed to initialize any connections: %v", initErrors) + } return nil }keychain-sdk/internal/tracker/tracker.go (1)
32-37: 🛠️ Refactor suggestionAdd threshold validation in the constructor.
The constructor should validate the threshold parameter to prevent issues with zero or very low values that could affect consensus.
func New(threshold uint8) *T { + if threshold == 0 { + panic("threshold must be greater than zero") + } return &T{ threshold: threshold, ingested: make(map[uint64]stringSet), } }
🧹 Nitpick comments (2)
keychain-sdk/tx_client_pool.go (1)
81-89: Implement load balancing when selecting a live transaction client.The
liveTxClientmethod currently returns the first live client found. To improve load distribution and fault tolerance, consider implementing a round-robin or random selection strategy among available live clients.func (cp *clientsPool) liveTxClient() (*client.TxClient, error) { + var liveClients []*client.TxClient for _, appClient := range cp.clients { if state := appClient.query.Conn().GetState(); isOnline(state) { - return appClient.txClient, nil + liveClients = append(liveClients, appClient.txClient) } } - return nil, errors.New("all node clients are down") + if len(liveClients) == 0 { + return nil, errors.New("all node clients are down") + } + + // Simple round-robin using a global counter (or could use random selection) + // For proper round-robin, you would need to store a counter in the pool + // and increment it each time you select a client + index := time.Now().UnixNano() % int64(len(liveClients)) // Simple pseudo-random selection + return liveClients[index], nil }keychain-sdk/internal/tracker/tracker.go (1)
60-62: Simplify threshold comparison to avoid unnecessary type conversions.The current threshold comparison involves two type conversions that are unnecessary. Since
len(value)returns anintandt.thresholdisuint8, it would be cleaner to compare them with a single type conversion.- if uint64(len(value)) < uint64(t.threshold) { + if len(value) < int(t.threshold) { return ActionSkip, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
keychain-sdk/internal/tracker/tracker.go(1 hunks)keychain-sdk/key_requests.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 (2)
- keychain-sdk/key_requests.go
- keychain-sdk/sign_requests.go
🧰 Additional context used
🧠 Learnings (2)
keychain-sdk/internal/tracker/tracker.go (3)
Learnt from: backsapc
PR: warden-protocol/wardenprotocol#810
File: keychain-sdk/internal/tracker/tracker.go:20-20
Timestamp: 2024-10-23T10:26:05.989Z
Learning: In the `keychain-sdk/internal/tracker/tracker.go` file, the variable `nodeUrl` should remain as `nodeUrl` and not be renamed to `nodeURL`.
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.
Learnt from: backsapc
PR: warden-protocol/wardenprotocol#810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The `MarkProcessed` method has been removed from `keychain-sdk/internal/tracker/status_tracker.go` in the Go codebase, as it is no longer needed.
keychain-sdk/tx_client_pool.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.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
keychain-sdk/tx_client_pool.go (1)
102-102: Great job fixing the typo in error messages.The error message correctly uses "acquire" instead of "aquire", addressing the issue mentioned in previous reviews.
keychain-sdk/internal/tracker/tracker.go (2)
15-24: Good implementation of the string set with a clear add method.The
stringSettype with theaddmethod is a memory-efficient way to implement a set in Go. The method correctly checks for existence and returns whether the item was newly added, which is a useful pattern.
50-65: Good implementation of the Ingest method with proper locking and threshold check.The method correctly uses a write lock since it modifies the map and the
stringSet. It also correctly returns different actions based on the ingestion status and threshold comparison.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
keychain-sdk/keychain.go (1)
74-85:⚠️ Potential issueFix potential panic with channel closure
Currently, channels are closed on function return (via
defer) while goroutines continue writing to them, which will cause a panic if the goroutines try to send to closed channels.Apply this diff to fix the issue:
keyRequestsCh := make(chan *wardentypes.KeyRequest) -defer close(keyRequestsCh) for _, appClient := range a.clientsPool.clients { go a.ingestKeyRequests(ctx, keyRequestsCh, appClient) } signRequestsCh := make(chan *wardentypes.SignRequest) -defer close(signRequestsCh) for _, appClient := range a.clientsPool.clients { go a.ingestSignRequests(ctx, signRequestsCh, appClient) }The goroutines should exit when the context is canceled, making explicit channel closure unnecessary.
keychain-sdk/internal/tracker/tracker.go (1)
32-37: 🛠️ Refactor suggestionAdd threshold validation to prevent unexpected behavior
The constructor doesn't validate the threshold parameter. A zero threshold would allow immediate processing of all requests, potentially bypassing consensus requirements.
Apply this diff to add validation:
func New(threshold uint8) *T { + if threshold == 0 { + panic("threshold must be greater than zero") + } return &T{ threshold: threshold, ingested: make(map[uint64]stringSet), } }
🧹 Nitpick comments (1)
keychain-sdk/internal/tracker/tracker.go (1)
39-54: Simplify threshold comparison logicThe current comparison converts both sides to uint64, which is unnecessary and could potentially cause issues with large values.
Apply this diff to simplify the comparison:
- if uint64(len(value)) < uint64(t.threshold) { + if len(value) < int(t.threshold) { return ActionSkip, nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/wardenkms/wardenkms.go(5 hunks)keychain-sdk/internal/tracker/tracker.go(1 hunks)keychain-sdk/internal/writer/writer.go(6 hunks)keychain-sdk/keychain.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/wardenkms.go
- keychain-sdk/tx_client_pool.go
- keychain-sdk/internal/writer/writer.go
🧰 Additional context used
🧠 Learnings (2)
keychain-sdk/keychain.go (2)
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.
Learnt from: backsapc
PR: warden-protocol/wardenprotocol#810
File: keychain-sdk/keychain.go:74-81
Timestamp: 2024-10-24T13:03:23.718Z
Learning: The team prefers not to refactor the `writer.New` constructor to use functional options in `keychain-sdk/keychain.go`.
keychain-sdk/internal/tracker/tracker.go (3)
Learnt from: backsapc
PR: warden-protocol/wardenprotocol#810
File: keychain-sdk/internal/tracker/tracker.go:20-20
Timestamp: 2024-10-23T10:26:05.989Z
Learning: In the `keychain-sdk/internal/tracker/tracker.go` file, the variable `nodeUrl` should remain as `nodeUrl` and not be renamed to `nodeURL`.
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.
Learnt from: backsapc
PR: warden-protocol/wardenprotocol#810
File: keychain-sdk/internal/tracker/status_tracker.go:0-0
Timestamp: 2024-10-24T13:03:19.344Z
Learning: The `MarkProcessed` method has been removed from `keychain-sdk/internal/tracker/status_tracker.go` in the Go codebase, as it is no longer needed.
🧬 Code Graph Analysis (1)
keychain-sdk/keychain.go (4)
cmd/wardenkms/wardenkms.go (1)
Config(23-44)keychain-sdk/config.go (1)
Config(11-58)keychain-sdk/internal/tracker/tracker.go (1)
New(32-37)keychain-sdk/internal/writer/writer.go (1)
New(41-53)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: lint
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
keychain-sdk/keychain.go (7)
32-34: Approve enhanced client management architectureThe refactoring to use a clients pool instead of a single client is aligned with the PR objectives to support multiple node endpoints.
40-41: Approve threshold-based trackers initializationThe trackers now use the ConsensusNodeThreshold configuration parameter, which properly integrates with the multi-node architecture described in the PR objectives.
59-64: LGTM: Proper clients pool initializationThe code correctly initializes the clients pool and handles potential connection errors.
66-72: LGTM: Well-structured writer initializationThe transaction writer is properly initialized with the necessary configuration parameters.
89-90: LGTM: Appropriate writer configurationThe transaction writer now correctly starts with the clients pool, allowing it to use any available client for transaction submission.
109-110: LGTM: Enhanced connection state reportingThe connection state method now returns states for all clients, which aligns with the PR objective to support multiple node endpoints.
113-118: LGTM: Simplified logger initializationThe logger initialization is streamlined while maintaining functionality.
keychain-sdk/internal/tracker/tracker.go (5)
8-13: LGTM: Clear action enumerationThe Action enum provides a clear and type-safe way to represent the possible outcomes of ingestion.
15-24: LGTM: Well-implemented set abstractionThe stringSet type with its add method provides a clean abstraction for managing unique ingester IDs.
26-30: LGTM: Threshold-based tracking designThe T struct now includes a threshold field to support consensus-based request processing, aligning with the PR objectives.
56-61: LGTM: Proper cleanup implementationThe Done method correctly cleans up the tracker state for completed requests.
63-72: LGTM: Clean helper function implementationThe ingestTracker helper method properly handles the creation and retrieval of tracker state.
Pitasi
left a comment
Pitasi
left a comment
There was a problem hiding this comment.
leaving some comments, mostly i'm concerned about the duplicate requests being written to the channels (both key requests and sign requests)
| value := t.ingestTracker(id) | ||
|
|
||
| if !value.add(ingesterId) { | ||
| return ActionSkip, errors.New("already ingested") |
There was a problem hiding this comment.
this is not an error, as it is expected to ingest the same id multiple times from the same source
| } | ||
|
|
||
| func (w *W) Start(ctx context.Context, flushErrors chan error) error { | ||
| func (w *W) Start(ctx context.Context, client SyncTxClient, flushErrors chan error) error { |
There was a problem hiding this comment.
why removing the Client field from W to add it back as a parameter here (and in the other methods)?
| func (a *wardenClient) keyRequests(ctx context.Context, batchSize int, keychainId uint64) ([]*wardentypes.KeyRequest, error) { | ||
| return a.query.PendingKeyRequests(ctx, &client.PageRequest{Limit: uint64(batchSize)}, keychainId) | ||
| } |
There was a problem hiding this comment.
if this is a method of wardenClient, let's move it near its definition (in tx_client_pool.go)
| func (a *wardenClient) signRequests(ctx context.Context, batchSize int, keychainId uint64) ([]*wardentypes.SignRequest, error) { | ||
| return a.query.PendingSignRequests(ctx, &client.PageRequest{Limit: uint64(batchSize)}, keychainId) | ||
| } |
There was a problem hiding this comment.
if this is a method of wardenClient, let's move it near its definition (in tx_client_pool.go)
| rw sync.RWMutex | ||
| ingested map[uint64]struct{} | ||
| threshold uint8 | ||
| rw sync.RWMutex |
There was a problem hiding this comment.
looks like we no longer need to use a RWMutex, a simple Mutex will do
|
|
||
| type stringSet map[string]struct{} | ||
|
|
||
| // add safely adds a string to the set. |
There was a problem hiding this comment.
there's no "safety" here, i suggest we rephrase it like this:
| // add safely adds a string to the set. | |
| // add adds a string to the set and returns true if the key was already present. |
| } | ||
|
|
||
| if action == tracker.ActionProcess { | ||
| keyRequestsCh <- keyRequest |
There was a problem hiding this comment.
duplicate keyRequests should NOT be written to keyRequestsCh, but this is not being guaranteed:
- N goroutines are running ingestRequest() concurrently
- threshold will most likely be set to T < N, e.g. T = 1
- all the N goroutines at some point will Ingest the same keyRequest, and all will write the same request to keyRequestsCh
Pitasi
left a comment
Pitasi
left a comment
There was a problem hiding this comment.
leaving some comments, mostly i'm concerned about the duplicate requests being written to the channels (both key requests and sign requests)
3 participants
#683