feat(zetaclient): add dedicated restricted addresses config file#3600
feat(zetaclient): add dedicated restricted addresses config file#3600
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe PR updates the client to load and monitor a restricted addresses configuration. The main application flow now initializes the restricted addresses settings after establishing the application context and before telemetry starts. A file watcher is registered to detect changes on the configuration file, with updated error logging and debug-level messages. Additionally, test cases across Bitcoin, EVM, Solana, and compliance modules are updated to use the new restricted addresses method, and the startup script is modified to initialize the restricted addresses file. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as ZetaClientd
participant Config as Config Manager
participant FS as File Watcher (fsnotify)
Client->>Config: Load restricted addresses config
alt Loading fails
Config-->>Client: Return error and log the issue
else Loading succeeds
Config-->>Client: Return restricted addresses data
Client->>FS: Register file watcher for config updates
end
FS->>Client: Notify on config file change
Client->>Config: Reload restricted addresses config
Config-->>Client: Apply updated configuration
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
gartnera
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3600 +/- ##
===========================================
- Coverage 64.70% 64.55% -0.15%
===========================================
Files 466 466
Lines 32462 32536 +74
===========================================
Hits 21003 21003
- Misses 10504 10578 +74
Partials 955 955
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
zetaclient/types/event_test.go (1)
66-66: Change correctly applies the new configuration approach.The update to use
SetRestrictedAddressesFromConfigis consistent with the project-wide change to use a dedicated restricted addresses configuration.Note: There appears to be a pre-existing typo in the function name
Test_Catetory(line 61) which should beTest_Category. Consider fixing this in a future PR.zetaclient/compliance/compliance_test.go (1)
26-26: Consider centralizing the repeated call in a test helper.
config.SetRestrictedAddressesFromConfig(cfg)is invoked multiple times across sub-tests. You can simplify the test code by extracting this into a shared initialization block or helper function.zetaclient/config/config.go (1)
55-57: Parameter rename and new field assignment.
Renaming the parameter frompathtobasePathclarifies intent. Settingcfg.ZetaCoreHome = basePathintegrates the new field into the loaded config flow.Also applies to: 86-86
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 55-55: zetaclient/config/config.go#L55
Added line #L55 was not covered by tests
[warning] 57-57: zetaclient/config/config.go#L57
Added line #L57 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/zetaclientd/start.go(2 hunks)contrib/localnet/scripts/start-zetaclientd.sh(1 hunks)zetaclient/chains/bitcoin/observer/event_test.go(2 hunks)zetaclient/chains/bitcoin/signer/outbound_data_test.go(1 hunks)zetaclient/chains/evm/observer/inbound_test.go(4 hunks)zetaclient/chains/evm/observer/outbound_test.go(1 hunks)zetaclient/chains/solana/observer/inbound_test.go(2 hunks)zetaclient/compliance/compliance_test.go(1 hunks)zetaclient/config/config.go(4 hunks)zetaclient/config/types.go(1 hunks)zetaclient/types/event_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- zetaclient/config/types.go
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.go`: Review the Go code, point out issues relative to ...
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/bitcoin/signer/outbound_data_test.gozetaclient/chains/bitcoin/observer/event_test.gozetaclient/chains/evm/observer/inbound_test.gozetaclient/chains/evm/observer/outbound_test.gocmd/zetaclientd/start.gozetaclient/config/config.gozetaclient/chains/solana/observer/inbound_test.gozetaclient/compliance/compliance_test.gozetaclient/types/event_test.go
`**/*.sh`: Review the shell scripts, point out issues relati...
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
contrib/localnet/scripts/start-zetaclientd.sh
🪛 GitHub Check: codecov/patch
zetaclient/config/config.go
[warning] 55-55: zetaclient/config/config.go#L55
Added line #L55 was not covered by tests
[warning] 57-57: zetaclient/config/config.go#L57
Added line #L57 was not covered by tests
[warning] 86-86: zetaclient/config/config.go#L86
Added line #L86 was not covered by tests
[warning] 92-92: zetaclient/config/config.go#L92
Added line #L92 was not covered by tests
[warning] 96-102: zetaclient/config/config.go#L96-L102
Added lines #L96 - L102 were not covered by tests
[warning] 105-114: zetaclient/config/config.go#L105-L114
Added lines #L105 - L114 were not covered by tests
[warning] 116-125: zetaclient/config/config.go#L116-L125
Added lines #L116 - L125 were not covered by tests
[warning] 129-134: zetaclient/config/config.go#L129-L134
Added lines #L129 - L134 were not covered by tests
[warning] 139-147: zetaclient/config/config.go#L139-L147
Added lines #L139 - L147 were not covered by tests
[warning] 152-157: zetaclient/config/config.go#L152-L157
Added lines #L152 - L157 were not covered by tests
[warning] 159-164: zetaclient/config/config.go#L159-L164
Added lines #L159 - L164 were not covered by tests
[warning] 166-169: zetaclient/config/config.go#L166-L169
Added lines #L166 - L169 were not covered by tests
[warning] 171-172: zetaclient/config/config.go#L171-L172
Added lines #L171 - L172 were not covered by tests
[warning] 176-177: zetaclient/config/config.go#L176-L177
Added lines #L176 - L177 were not covered by tests
[warning] 180-185: zetaclient/config/config.go#L180-L185
Added lines #L180 - L185 were not covered by tests
[warning] 187-191: zetaclient/config/config.go#L187-L191
Added lines #L187 - L191 were not covered by tests
[warning] 216-217: zetaclient/config/config.go#L216-L217
Added lines #L216 - L217 were not covered by tests
🔇 Additional comments (23)
zetaclient/chains/bitcoin/signer/outbound_data_test.go (1)
28-28: Clean implementation of the new configuration approach.The change from
LoadComplianceConfigtoSetRestrictedAddressesFromConfigaligns well with the PR objective of introducing a dedicated restricted addresses configuration file.contrib/localnet/scripts/start-zetaclientd.sh (1)
119-120: Good defensive initialization of the restricted addresses config file.Initializing the configuration file with an empty JSON array prevents potential parsing errors and log spam, eliminating the need to check for file existence in the application code.
zetaclient/chains/evm/observer/inbound_test.go (1)
299-299: Consistent application of the new restricted addresses configuration approach.All instances of loading compliance configuration have been appropriately updated to use the new dedicated method for setting restricted addresses from configuration.
Also applies to: 306-306, 313-313, 346-346, 353-353, 403-403, 417-417
cmd/zetaclientd/start.go (2)
61-68: Good implementation of restricted addresses config loading with proper error handling.The code correctly handles the error case when loading the restricted addresses config, and properly registers a watcher to monitor changes to the config file. This aligns well with the PR objective of introducing a dedicated config file that can be monitored for changes without restarting the service.
89-89: Appropriate logging level change from Info to Debug.Changing the log level from Info to Debug for configuration updates is a good improvement since this is diagnostic information rather than operational information. This also addresses the previous comment about not logging the whole configuration at startup, reducing log verbosity.
zetaclient/chains/evm/observer/outbound_test.go (1)
73-73: Function call updated to use new restricted addresses mechanism.The implementation correctly updates the function call from
LoadComplianceConfigtoSetRestrictedAddressesFromConfigto align with the new restricted addresses configuration approach. This ensures that tests correctly validate the compliance restrictions.zetaclient/chains/bitcoin/observer/event_test.go (2)
50-50: Function call updated to use new restricted addresses mechanism.The implementation correctly updates the function call from
LoadComplianceConfigtoSetRestrictedAddressesFromConfigto align with the new restricted addresses configuration approach.
318-318: Function call updated to use new restricted addresses mechanism.The implementation correctly updates the function call from
LoadComplianceConfigtoSetRestrictedAddressesFromConfig, maintaining consistency with the new restricted addresses configuration approach.zetaclient/chains/solana/observer/inbound_test.go (2)
156-156: Function call updated to use new restricted addresses mechanism.The implementation correctly updates the function call from
LoadComplianceConfigtoSetRestrictedAddressesFromConfigto align with the new restricted addresses configuration approach.
176-176: Function call updated to use new restricted addresses mechanism.The implementation correctly updates the function call from
LoadComplianceConfigtoSetRestrictedAddressesFromConfig, maintaining consistency across all test files with the new restricted addresses configuration approach.zetaclient/compliance/compliance_test.go (4)
31-31: Consider centralizing the repeated call in a test helper.
Same recommendation as above for reducing duplication and improving maintainability.
37-37: Same repeated invocation context.
Consolidating this call can make tests more concise.
42-42: Same repeated invocation context.
Extracting this to a common setup can keep tests DRY (Don’t Repeat Yourself).
48-48: Same repeated invocation context.
Unifying setup logic promotes readability and consistency.zetaclient/config/config.go (9)
5-5: New imports support watchers and concurrency.
These additions (context, sync, fsnotify, and zerolog) are well-suited for handling file system events and logging in a concurrent environment.Also applies to: 11-11, 13-13, 15-15
20-20: Lock introduced for concurrency control.
restrictedAddressBookLockhelps ensure thread-safe updates to the global map. Confirm that all access paths consistently use the lock to avoid data races.
22-22: Constant for restricted addresses path.
DefiningrestrictedAddressesPathas a constant improves maintainability and avoids hardcoded strings.
91-92: Selective loading of restricted addresses.
SetRestrictedAddressesFromConfigreplaces a broader compliance-loading function, focusing on restricted addresses only. This separation of concerns seems more maintainable.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 92-92: zetaclient/config/config.go#L92
Added line #L92 was not covered by tests
96-102: Utility for constructing absolute paths.
getRestrictedAddressAbsPathproperly handles directory resolution. Verify that permission errors and invalid paths are handled or surfaced to the caller.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 96-102: zetaclient/config/config.go#L96-L102
Added lines #L96 - L102 were not covered by tests
105-125: Atomic refresh of restricted addresses.
loadRestrictedAddressesConfigreloads addresses under lock, merging from the main config and the dedicated file. This approach prevents interleaving writes. Good concurrency practice.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 105-114: zetaclient/config/config.go#L105-L114
Added lines #L105 - L114 were not covered by tests
[warning] 116-125: zetaclient/config/config.go#L116-L125
Added lines #L116 - L125 were not covered by tests
129-134: Convenience loader for restricted addresses.
LoadRestrictedAddressesConfigimproves clarity by encapsulating the path resolution and loading logic.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-134: zetaclient/config/config.go#L129-L134
Added lines #L129 - L134 were not covered by tests
139-194: Robust file monitoring for address updates.
WatchRestrictedAddressesConfigusesfsnotifyto detect changes and reload. Consider handling transient file write states or partial writes by adding a brief retry or delay before reading.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 139-147: zetaclient/config/config.go#L139-L147
Added lines #L139 - L147 were not covered by tests
[warning] 152-157: zetaclient/config/config.go#L152-L157
Added lines #L152 - L157 were not covered by tests
[warning] 159-164: zetaclient/config/config.go#L159-L164
Added lines #L159 - L164 were not covered by tests
[warning] 166-169: zetaclient/config/config.go#L166-L169
Added lines #L166 - L169 were not covered by tests
[warning] 171-172: zetaclient/config/config.go#L171-L172
Added lines #L171 - L172 were not covered by tests
[warning] 176-177: zetaclient/config/config.go#L176-L177
Added lines #L176 - L177 were not covered by tests
[warning] 180-185: zetaclient/config/config.go#L180-L185
Added lines #L180 - L185 were not covered by tests
[warning] 187-191: zetaclient/config/config.go#L187-L191
Added lines #L187 - L191 were not covered by tests
216-217: Thread-safe lookups.
The read lock aroundrestrictedAddressBookensures concurrent reads won't conflict with writes. Kudos for consistent locking discipline.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 216-217: zetaclient/config/config.go#L216-L217
Added lines #L216 - L217 were not covered by tests
gartnera
force-pushed
the
zetaclient-restricted-config
branch
from
30ef13c to
a59a37d
Compare
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
ws4charlie
left a comment
ws4charlie
left a comment
There was a problem hiding this comment.
overall looks good. left minor comment
gartnera
force-pushed
the
zetaclient-restricted-config
branch
from
33a229b to
5ee9307
Compare
* feat(zetaclient): add dedicated config file restricted addresses * use errors * use bg. errors should not be fatal to the main prodcess * changelog
* feat(zetaclient): add dedicated config file restricted addresses * use errors * use bg. errors should not be fatal to the main prodcess * changelog
…) (#3614) * feat(zetaclient): add dedicated config file restricted addresses * use errors * use bg. errors should not be fatal to the main prodcess * changelog
3 participants
Add a dedicated restricted addresses config file. Watch the file for changes and automatically reload it when it changes. This allows us to easily update the addresses in an automated sidecar process without having to restart zetaclient and increase error rates.
You can still use the
RestrictedAddressessection of the main config. The addresses from the new config will be merged with those addresses.Summary by CodeRabbit
New Features
Chores
Refactor
Tests