1935 fix concurrent writes #1936
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… function; change to RWMutex; add getter; add test
Contributor
## Walkthrough
The changes update the Quicksilver dependency version in `icq-relayer`, remove it from `xcclookup`, and reorganize dependencies there. In `xcclookup`, a read-write mutex was added to the `Response` struct to ensure thread-safe access, with new getter methods returning deep copies. Unit tests were added to verify mutex presence and thread safety during concurrent access. A test was also refined to assert non-nil key fields.
## Changes
| File(s) | Change Summary |
|----------------------------------------------|---------------------------------------------------------------------------------------------------------------------|
| icq-relayer/go.mod | Updated `github.com/quicksilver-zone/quicksilver` dependency from `v1.8.0-beta.11` to `v1.9.0`. |
| xcclookup/go.mod | Removed `github.com/quicksilver-zone/quicksilver` and several cloud, telemetry, and other dependencies; added new testing and utility libs. |
| xcclookup/pkgs/types/rpc.go | Added `sync.RWMutex` to `Response` struct; modified `Update` method to use mutex; added thread-safe getters returning deep copies. |
| xcclookup/pkgs/types/rpc_test.go | Added tests verifying mutex presence and thread-safe concurrent access to `Response` assets. |
| xcclookup/pkgs/types/output_test.go | Modified test to assert non-nil `Messages` and `Assets` fields individually instead of whole struct. |
| xcclookup/pkgs/services/assets_service_test.go | Changed message access in test from direct field to getter method `GetMessages()`. |
| xcclookup/pkgs/types/output.go | Updated `outputResponse` to handle nil responses safely and marshal a thread-safe JSON copy of `Response`. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Test as Test (rpc_test.go)
participant Resp as Response (with RWMutex)
loop Concurrent Goroutines
Test->>+Resp: GetAssets()
Resp-->>-Test: Deep copy of Assets (thread-safe)
endEstimated code review effort2 (~20 minutes) Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Learnt from: joe-bowman Learnt from: joe-bowman Learnt from: joe-bowman Learnt from: joe-bowman Learnt from: joe-bowman Learnt from: joe-bowman |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
xcclookup/pkgs/types/rpc_test.go (2)
11-23: Basic functionality test is good but could be more specific.The test verifies that GetAssets works, but the test name suggests it should verify mutex existence. Consider renaming or adding an explicit mutex field check.
30-32: Consider using thread-safe initialization in tests.The test directly assigns to
response.Assets["chain1"]which bypasses the thread-safe Update method. Consider using the Update method or adding a comment explaining why direct assignment is used in tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
icq-relayer/go.mod(1 hunks)xcclookup/go.mod(0 hunks)xcclookup/pkgs/types/rpc.go(2 hunks)xcclookup/pkgs/types/rpc_test.go(1 hunks)
🧠 Learnings (3)
📓 Common learnings
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1821
File: icq-relayer/go.mod:214-214
Timestamp: 2025-02-16T19:50:23.555Z
Learning: The icq-relayer daemon in the Quicksilver project requires using the local version of the quicksilver module (`../`) via the `replace` directive in its go.mod, as it needs to always use the current version from the same repository.
icq-relayer/go.mod (1)
Learnt from: joe-bowman
PR: #1821
File: icq-relayer/go.mod:214-214
Timestamp: 2025-02-16T19:50:23.555Z
Learning: The icq-relayer daemon in the Quicksilver project requires using the local version of the quicksilver module (../) via the replace directive in its go.mod, as it needs to always use the current version from the same repository.
xcclookup/pkgs/types/rpc_test.go (1)
Learnt from: arhamchordia
PR: #1803
File: x/supply/keeper/keeper_test.go:31-66
Timestamp: 2025-01-28T19:03:57.668Z
Learning: Avoid suggesting test cases that duplicate functionality already tested in msg_server_test.go, particularly for module account operations and balance transfers. Focus on unique keeper functionality instead.
💤 Files with no reviewable changes (1)
- xcclookup/go.mod
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: joe-bowman
PR: quicksilver-zone/quicksilver#1821
File: icq-relayer/go.mod:214-214
Timestamp: 2025-02-16T19:50:23.555Z
Learning: The icq-relayer daemon in the Quicksilver project requires using the local version of the quicksilver module (`../`) via the `replace` directive in its go.mod, as it needs to always use the current version from the same repository.
icq-relayer/go.mod (1)
Learnt from: joe-bowman
PR: #1821
File: icq-relayer/go.mod:214-214
Timestamp: 2025-02-16T19:50:23.555Z
Learning: The icq-relayer daemon in the Quicksilver project requires using the local version of the quicksilver module (../) via the replace directive in its go.mod, as it needs to always use the current version from the same repository.
xcclookup/pkgs/types/rpc_test.go (1)
Learnt from: arhamchordia
PR: #1803
File: x/supply/keeper/keeper_test.go:31-66
Timestamp: 2025-01-28T19:03:57.668Z
Learning: Avoid suggesting test cases that duplicate functionality already tested in msg_server_test.go, particularly for module account operations and balance transfers. Focus on unique keeper functionality instead.
🔇 Additional comments (3)
xcclookup/pkgs/types/rpc.go (3)
63-63: Excellent addition of thread-safe mutex.The RWMutex is properly added with JSON exclusion tag, enabling safe concurrent access to the Response struct.
68-69: Proper implementation of write lock in Update method.The mutex locking is correctly implemented using write locks for exclusive access during updates to Messages and Assets fields.
91-102: Well-implemented thread-safe getter for Assets.The GetAssets method correctly uses read locks and creates a deep copy to prevent race conditions. The deep copy implementation properly handles the slice of Asset structs.
…ex guarded getters and setters, to ensure thread-safety. Add new struct for JSON marshalling that doesn't include the mutex. Update instance where test was comparing response objects, which is now not possible because of copying locks, so we compare individual values
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1936 +/- ##
=======================================
Coverage 57.63% 57.63%
=======================================
Files 172 172
Lines 15268 15268
=======================================
Hits 8800 8800
Misses 5702 5702
Partials 766 766
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ajansari95
approved these changes
Aug 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
1. Summary
Fixes #1935
2.Type of change
3. Implementation details
Summary by CodeRabbit
Bug Fixes
Tests
Chores