refactor: organize time-related consts #60
Conversation
📝 WalkthroughWalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Config
participant StreamMgr
participant Notifier
participant PartyCoord
App->>Config: Retrieve timeout/resource constants
App->>StreamMgr: Initialize with config values
App->>Notifier: Initialize with config TTLs and intervals
App->>PartyCoord: Initialize with config timeouts and retry intervals
StreamMgr->>Config: Use timeouts for stream operations
Notifier->>Config: Use TTL and cleanup intervals
PartyCoord->>Config: Use timeouts and retry intervals
Note over App,Config: All timing and sizing parameters are centralized
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
p2p/party_coordinator.go (4)
47-50: Good defensive defaulting – nit: consider lifting the magic number.The fallback to
10 * time.Secondis sensible, but putting the value inconfig(e.g.config.DefaultPartyTimeout) would keep all time-related knobs in one place, matching the PR goal.
58-59: Double-default overlap.
NewStreamManageralready assignstime.MinutewhenmaxAgeBeforeCleanup == 0.
Becauseconfig.StreamExcessTTLis exactly one minute, passing that constant buys no functional gain while coupling two defaults. Either:
- Remove the defaulting inside
NewStreamManager, or- Omit the argument here (
0) and rely on the constructor default.Keeping a single source of truth avoids accidental drift.
323-337: Retry loop: consider context cancellation for clarity.The
donechannel plustickerworks, but Go’s idiomatic pattern is acontext.ContextwithWithTimeout/WithCancel.
Doing so removes the explicitdonechoreography and integrates naturally withpc.stopChanviacontext.AfterFunc.Not blocking this PR, but worth a follow-up refactor.
380-389: Control-flow rewritten – retain explicit else-branches for readability.The new
switchis neat, yet combining error conditions and success in a single block can make future maintenance harder.
A small restructure keeps intent crystal clear:if err != nil { return nil, errors.Wrap(err, "failed to parse peer ids") } if len(pIDs) < peerGroup.threshold { return pIDs, errors.New("not enough peers") } if peerGroup.getLeaderResponse().Type == messages.JoinPartyLeaderComm_Success { return pIDs, nil } return pIDs, ErrJoinPartyTimeoutNot mandatory, but marginally more readable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
config/const.go(1 hunks)keygen/ecdsa/tss_keygen.go(2 hunks)keygen/eddsa/tss_keygen.go(2 hunks)keysign/ecdsa/tss_keysign.go(2 hunks)keysign/eddsa/tss_keysign.go(2 hunks)keysign/notifier.go(2 hunks)keysign/signature_notifier.go(5 hunks)keysign/signature_notifier_test.go(2 hunks)p2p/communication.go(8 hunks)p2p/party_coordinator.go(8 hunks)p2p/stream_manager.go(7 hunks)p2p/stream_manager_test.go(1 hunks)tss/keysign.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (10)
keygen/ecdsa/tss_keygen.go (1)
config/const.go (1)
TSSCommonFinalTimeout(40-40)
keysign/signature_notifier_test.go (1)
config/const.go (1)
SigNotifierTTL(36-36)
keygen/eddsa/tss_keygen.go (1)
config/const.go (1)
TSSCommonFinalTimeout(40-40)
keysign/eddsa/tss_keysign.go (1)
config/const.go (1)
TSSCommonFinalTimeout(40-40)
keysign/notifier.go (1)
config/const.go (1)
SigNotifierTTL(36-36)
keysign/ecdsa/tss_keysign.go (1)
config/const.go (1)
TSSCommonFinalTimeout(40-40)
keysign/signature_notifier.go (2)
p2p/stream_manager.go (1)
NewStreamManager(42-66)config/const.go (5)
StreamExcessTTL(24-24)SigNotifierCleanupInterval(37-37)SigNotifierTTL(36-36)StreamTimeoutConnect(15-15)SigNotifierAckTimeout(33-33)
tss/keysign.go (1)
p2p/party_coordinator.go (3)
ErrSigReceived(27-27)NotificationSigReceived(22-22)ErrNotActiveSigner(28-28)
p2p/stream_manager.go (2)
config/const.go (3)
StreamTimeoutRead(18-18)StreamMaxPayload(27-27)StreamTimeoutWrite(21-21)p2p/protocol.go (1)
PayloadHeaderLen(36-36)
p2p/party_coordinator.go (5)
p2p/stream_manager.go (1)
NewStreamManager(42-66)config/const.go (3)
StreamExcessTTL(24-24)StreamTimeoutConnect(15-15)PartyJoinMemberRetryInterval(42-42)logs/fields.go (2)
MsgID(8-8)Leader(11-11)conversion/key_provider.go (1)
GetPubKeyFromPeerID(78-97)messages/join_party.pb.go (1)
JoinPartyLeaderComm_Success(27-27)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test go 1.22
🔇 Additional comments (49)
keygen/ecdsa/tss_keygen.go (2)
18-18: Appropriate import addition.The import of the config package is correctly added to support the centralized timeout constants refactoring.
155-155: Good refactor: Hardcoded timeout replaced with configurable constant.Replacing the hardcoded timeout with
config.TSSCommonFinalTimeoutimproves maintainability by centralizing timeout configuration. This change maintains the same timeout value (5 seconds) while making it configurable from a single location.p2p/stream_manager_test.go (1)
152-152: Test updated to match new constructor signature.The test has been correctly updated to accommodate the additional
maxAgeBeforeCleanupparameter now required by theNewStreamManagerconstructor. The value of0is used for the duration in this test context.Verify that using
0as the duration value is the intended behavior for testing. While this will pass the test, consider if a more realistic value would provide better test coverage for stream cleanup functionality.keysign/notifier.go (2)
17-18: Import block properly organized.The import for the config package has been correctly added and the trailing closing parenthesis has been properly fixed, resolving a syntax issue in the import block.
51-51: Good refactor: Hardcoded TTL replaced with configurable constant.Replacing the hardcoded TTL value with
config.SigNotifierTTLimproves maintainability and consistency. This change centralizes the configuration while maintaining the same functional behavior (30 seconds TTL).keygen/eddsa/tss_keygen.go (2)
17-17: Appropriate import addition.The import of the config package is correctly added to support the centralized timeout constants refactoring.
146-146: Good refactor: Hardcoded timeout replaced with configurable constant.Replacing the hardcoded timeout with
config.TSSCommonFinalTimeoutimproves maintainability by centralizing timeout configuration. This change maintains the same timeout value (5 seconds) while making it configurable from a single location. The change is consistent with similar refactoring in the ECDSA keygen implementation.keysign/ecdsa/tss_keysign.go (2)
22-23: Appropriate config package addition.Clean addition of the config package import to support the use of centralized timeout constants.
210-215: Good replacement of hardcoded timeout with configurable constant.This change improves maintainability by replacing the hardcoded 5-second timeout with a centralized constant. The
config.TSSCommonFinalTimeoutvalue matches the previous behavior while providing a single point of configuration for future adjustments.keysign/eddsa/tss_keysign.go (2)
22-23: Appropriate config package addition.Good addition of the config package import to support centralized timeout constants.
197-202: Good replacement of hardcoded timeout with configurable constant.This change properly replaces the hardcoded 5-second timeout with the centralized
config.TSSCommonFinalTimeoutconstant, ensuring consistent timeout behavior across the codebase.keysign/signature_notifier_test.go (2)
18-19: Appropriate config package addition.The config package import is correctly added to access the centralized TTL constant.
167-168: Good test adaptation to use centralized TTL constant.This change correctly updates the test to validate against
config.SigNotifierTTLinstead of a hardcoded value, ensuring tests remain aligned with implementation.tss/keysign.go (3)
99-101: Corrected error constant usage.The error constant has been properly standardized to use
p2p.ErrSigReceivedinstead ofp2p.ErrSignReceived, ensuring consistency across the codebase.
330-336: Good standardization of notification constant.The string literal
"signature received"has been appropriately replaced with the constantp2p.NotificationSigReceived, improving maintainability and consistency.
363-366: Corrected error constant in conditional check.The error check has been properly updated to use the standardized
p2p.ErrSigReceivedconstant instead ofp2p.ErrSignReceived, ensuring consistent error handling.p2p/stream_manager.go (9)
17-17: Good addition of the config packageAdding the config package import enables the use of centralized time-related constants, aligning with the refactoring goal.
21-21: Improved variable namingRenaming the constant from
unknowntounknownMsgIDenhances code readability by making the intent clearer and more descriptive.
42-47: Well-designed constructor enhancementThe constructor now accepts a configurable TTL parameter with a sensible default fallback. This increases flexibility while maintaining backward compatibility.
93-93: Consistent use of the renamed constantProperly updated the usage of
unknownMsgIDin theStashUnknownmethod, ensuring consistency throughout the codebase.
172-172: Consistent usage in cleanup methodProperly updated the usage of
unknownMsgIDin the cleanup method, maintaining code consistency.
207-207: Improved timeout configurationReplaced hardcoded read timeout with the centralized configuration constant, improving maintainability.
220-222: Improved payload size validationReplaced hardcoded maximum payload size with a centralized configuration constant, providing better consistency across the codebase.
236-238: Consistent payload limit applicationThe maximum payload size constraint is consistently applied using the same configuration constant as in the read path, ensuring uniform behavior.
240-240: Standardized write timeoutReplaced hardcoded write timeout with the centralized configuration constant, completing the standardization of all stream timeouts.
p2p/communication.go (7)
24-24: Good addition of the config packageAdding the config package import enables the use of centralized time-related constants, aligning with the refactoring goal.
90-90: Proper integration with the StreamManagerUpdated the constructor call to include the new TTL parameter from config, ensuring consistent stream lifecycle management.
145-146: Standardized connection timeoutReplaced hardcoded connection timeout with the centralized configuration constant, improving consistency across the codebase.
192-202: Enhanced stream reading timeoutImproved the timeout handling for stream reading using the centralized configuration constant, including properly referencing the timeout value in the log message.
274-274: Simplified resource limits configurationReplaced manual resource limit construction with the centralized
ScalingLimitsfunction, reducing code complexity and improving maintainability.
294-296: Cleaner host initializationSimplified the libp2p.New call by using a more concise parameter style, improving code readability.
356-357: Consistent connection timeout applicationUsed the same configuration constant for timeout in peer connections, ensuring uniform behavior across the codebase.
config/const.go (6)
1-11: Well-structured new configuration packageThe new config package is properly organized with clear imports and a descriptive package comment.
13-28: Comprehensive stream-related constantsWell-organized stream constants with descriptive comments. The timeout values are reasonable, and the max payload size is defined using bit shifting for clarity.
31-38: Well-defined signature notifier constantsClear organization of signature notifier constants with descriptive comments explaining their purpose.
40-42: Properly defined general timeout constantsAdded important general timeout constants with appropriate values for TSS operations and party join retry intervals.
45-76: Excellent resource limits functionThe
ScalingLimitsfunction is well-designed, providing a centralized and configurable way to define resource constraints for libp2p. The implementation properly handles protocol-specific limits and automatically scales based on system memory.
79-81: Useful utility functionThe
mbhelper function provides a clean way to convert megabytes to bytes, enhancing code readability when defining memory limits.keysign/signature_notifier.go (7)
16-16: Good addition of the config packageAdding the config package import enables the use of centralized time-related constants, aligning with the refactoring goal.
56-56: Proper integration with the StreamManagerUpdated the StreamManager initialization to use the standardized TTL from config, ensuring consistent stream lifecycle management.
81-97: Improved cleanup function structureEnhanced the cleanup logic with:
- Better function organization using a local function
- Proper deferred mutex unlocking
- Added deletion counter for better observability
- Improved conditional logging that only reports when actions were taken
These changes improve code maintainability and operational visibility.
99-99: Standardized cleanup intervalReplaced hardcoded cleanup interval with the centralized configuration constant, ensuring consistency across the codebase.
162-162: Standardized notifier TTLUpdated the notifier TTL to use the centralized configuration constant, maintaining consistent timeout behavior.
169-169: Standardized connection timeoutReplaced hardcoded connection timeout with the centralized configuration constant, ensuring consistency with other parts of the system.
211-213: Improved ACK timeout handlingEnhanced the ACK timeout implementation by:
- Using the standardized configuration constant
- Updating the comment to clearly explain the purpose
- Maintaining consistent code style with the rest of the codebase
This change improves code clarity and maintainability.
p2p/party_coordinator.go (4)
260-263: 👍 Config-driven connect timeout.Replacing the hard-coded
4 swithconfig.StreamTimeoutConnectaligns with the refactor goal and makes tuning easier.
358-360: Error semantics are now clear – good use of sentinel error.Return-early on
ErrSigReceivedsimplifies upstream handling.
411-413: Consistent signalling path confirmed.Leader side mirrors the member side and correctly propagates
ErrSigReceived.
16-23:✅ Verification successful
Rename introduces breaking-change risk; scan the repo for stale usages.
ErrSigReceivedandNotificationSigReceivedreplace the previousErrSignReceived/ literal.
Compilation will break wherever the old identifiers remain. Please verify the entire repo (tests, other packages, mocks).
🏁 Script executed:
#!/bin/bash # Find remaining references to the old identifiers / literal rg -n --hidden --glob '!.git' $'\bErrSignReceived\b|\"signature received\"'Length of output: 153
No stale references to ErrSignReceived remain
I ran a repository-wide search and confirmed there are no occurrences of
ErrSignReceivedor the literal"signature received"beyond its updated definition inp2p/party_coordinator.go. All references have been updated accordingly.
This PR refactors all hard-coded time/interval/ttl/duration and resource constants in
config/const.go.This unification makes the code more transparent and simpler. It also reuses values across similar code-paths.
Other improvements
sleep(500ms)Closes #56
Related zeta-chain/node#3843
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Tests