Refactor: Replace magic numbers with named constants and enums for clarity and maintainability #917
Conversation
parth-soni07
changed the title
|
|
||
| @dataclass |
There was a problem hiding this comment.
Hey @pacrob , thanks for the review, I realised this line was removed accidently and I have added it back.
libp2p/relay/circuit_v2/dcutr.py
Outdated
| @@ -47,11 +53,6 @@ | |||
| # Maximum message size for DCUtR (4KiB as per spec) | |||
| MAX_MESSAGE_SIZE = 4 * 1024 | |||
|
|
|||
| # Timeouts | |||
| STREAM_READ_TIMEOUT = 30 # seconds | |||
There was a problem hiding this comment.
This is being changed from 30 to 15. Are you sure that's appropriate here? Should be mentioned in a newsfragment if it is to be changed.
There was a problem hiding this comment.
Fixed this as well, there are two STREAM_READ_TIMEOUTS, whose value differed in dcutr & protocol files, so I have now defined them locally in the respective files in the new commit, should fix. Thanks!
|
Overall great idea to centralize constants, thanks @parth-soni07! Just a couple questions, and please add a newsfragment. |
|
@parth-soni07 : Thank you so much for your efforts and contribution on this PR. Reviewing it in detail. Wish if you could fix the CI/CD issue too. |
| # Role configuration (bit-flags) | ||
| roles: RelayRole = RelayRole.STOP | RelayRole.CLIENT | ||
|
|
||
| # Resource limits | ||
| limits: RelayLimits | None = None | ||
|
|
||
| # Discovery configuration | ||
| bootstrap_relays: list[PeerInfo] = field(default_factory=list) | ||
| min_relays: int = 3 | ||
| max_relays: int = 20 | ||
| discovery_interval: int = 300 # seconds | ||
| min_relays: int = DEFAULT_MIN_RELAYS | ||
| max_relays: int = DEFAULT_MAX_RELAYS | ||
| discovery_interval: int = DEFAULT_DISCOVERY_INTERVAL | ||
|
|
||
| # Connection configuration | ||
| reservation_ttl: int = 3600 # seconds | ||
| max_circuit_duration: int = 3600 # seconds | ||
| max_circuit_bytes: int = 1024 * 1024 * 1024 # 1GB | ||
| reservation_ttl: int = DEFAULT_RESERVATION_TTL | ||
| max_circuit_duration: int = DEFAULT_MAX_CIRCUIT_DURATION | ||
| max_circuit_bytes: int = DEFAULT_MAX_CIRCUIT_BYTES |
There was a problem hiding this comment.
Wouldn't it be better to make an __init__ function here and in the HopConfig and ClientConfig class, so that end user can also edit the configs if they want, otherwise set the default values in case nothing is specified? @parth-soni07 @pacrob @seetadev
There was a problem hiding this comment.
Yupp @lla-dane , having a look, I think that is a great idea.
| if len(self._discovered_relays) > MAX_RELAYS_TO_TRACK: | ||
| # Sort by last seen time and keep only the most recent ones | ||
| sorted_relays = sorted( | ||
| self._discovered_relays.items(), | ||
| key=lambda x: x[1].last_seen, | ||
| reverse=True, | ||
| ) | ||
| to_remove = sorted_relays[self.max_relays :] | ||
| to_remove = sorted_relays[MAX_RELAYS_TO_TRACK:] |
There was a problem hiding this comment.
Wouldn't it be safer to keep these self.max_relays only, in case the user tweaked the configs in runtime. @pacrob @parth-soni07 @seetadev
There was a problem hiding this comment.
Sure @lla-dane , I'll have a look at this & will push the changes accordingly along with the newsfragment. Thanks for your feedback!
|
@parth-soni07 : Neat effort. Could you please reply to @lla-dane 's feedback. Also, please add the newsfragment. |
|
@parth-soni07 : I will review the changes made in detail and will also add feedback to the points shared by @lla-dane. |
Sure @seetadev, will do. Also, thanks for your feedback! |
Overview?
This pull request refactors multiple modules in the codebase to eliminate the use of magic numbers and hardcoded values. Instead, all such values are now defined as named constants or enums. This change is aimed at improving code clarity, maintainability, and adherence to best practices.
Details
What was changed?
Named Constants
Enums
Code update
Impact
This is a non-breaking, internal refactor. All existing functionality remains unchanged.
All existing tests should continue to pass. No new tests are required, but this refactor lays the groundwork for easier future testing and configuration changes.