-
Notifications
You must be signed in to change notification settings - Fork 228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bump slinky & forwarding module dependencies #238
Conversation
Caution Review failedThe pull request is closed. WalkthroughWalkthroughRecent updates to the application enhance oracle data integration and vote extension handling. Key improvements include the introduction of an aggregator strategy for oracle data, updated event handling, and a new method for market map queries. These changes streamline functionality, improve error handling, and enhance overall performance, making the application more robust in processing oracle-related data. Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #238 +/- ##
=======================================
Coverage 39.85% 39.85%
=======================================
Files 259 259
Lines 24885 24952 +67
=======================================
+ Hits 9917 9945 +28
- Misses 13423 13463 +40
+ Partials 1545 1544 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.mod
is excluded by!**/*.mod
go.sum
is excluded by!**/*.sum
,!**/*.sum
Files selected for processing (2)
- app/app.go (5 hunks)
- app/oracle/client.go (1 hunks)
Additional context used
GitHub Check: codecov/patch
app/oracle/client.go
[warning] 213-222: app/oracle/client.go#L213-L222
Added lines #L213 - L222 were not covered by tests
[warning] 225-230: app/oracle/client.go#L225-L230
Added lines #L225 - L230 were not covered by tests
[warning] 232-232: app/oracle/client.go#L232
Added line #L232 was not covered by tests
Additional comments not posted (7)
app/oracle/client.go (2)
225-230
: Ensure the client is initialized before making the request.The method correctly checks if the client is initialized before proceeding. This is crucial to avoid nil pointer dereferences.
Tools
GitHub Check: codecov/patch
[warning] 225-230: app/oracle/client.go#L225-L230
Added lines #L225 - L230 were not covered by tests
232-232
: LGTM!The code changes are approved. The method correctly handles market map queries and incorporates metrics tracking and error handling.
Tools
GitHub Check: codecov/patch
[warning] 232-232: app/oracle/client.go#L232
Added line #L232 was not covered by testsapp/app.go (5)
1155-1161
: LGTM!The introduction of the
aggregatorFn
improves code readability and maintainability by encapsulating the median aggregation logic.
Line range hint
1162-1181
:
LGTM!The initialization of the
oraclePreBlockHandler
with the newaggregatorFn
and other dependencies enhances the handling of oracle data.
1182-1213
: LGTM!The introduction of the new vote extensions handler for extending and verifying vote extensions integrates the new
aggregatorFn
and other dependencies effectively.
1306-1306
: LGTM!The update to the
PreBlocker
method simplifies the flow of pre-block processing and improves error handling.
586-586
: LGTM!The update to the
ForwardingKeeper
initialization to include theruntime.ProvideEventService()
enhances the app's event handling capabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.mod
is excluded by!**/*.mod
go.sum
is excluded by!**/*.sum
,!**/*.sum
Files selected for processing (1)
- app/oracle/client.go (4 hunks)
Additional context used
GitHub Check: codecov/patch
app/oracle/client.go
[warning] 185-194: app/oracle/client.go#L185-L194
Added lines #L185 - L194 were not covered by tests
[warning] 209-209: app/oracle/client.go#L209
Added line #L209 was not covered by tests
[warning] 212-212: app/oracle/client.go#L212
Added line #L212 was not covered by tests
[warning] 223-232: app/oracle/client.go#L223-L232
Added lines #L223 - L232 were not covered by tests
[warning] 235-240: app/oracle/client.go#L235-L240
Added lines #L235 - L240 were not covered by tests
[warning] 242-242: app/oracle/client.go#L242
Added line #L242 was not covered by tests
[warning] 246-255: app/oracle/client.go#L246-L255
Added lines #L246 - L255 were not covered by tests
[warning] 258-263: app/oracle/client.go#L258-L263
Added lines #L258 - L263 were not covered by tests
[warning] 265-265: app/oracle/client.go#L265
Added line #L265 was not covered by tests
Additional comments not posted (1)
app/oracle/client.go (1)
219-243
: Add tests for theMarketMap
method.The
MarketMap
method is not covered by tests. Ensure that unit tests are added to verify its functionality.Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 223-232: app/oracle/client.go#L223-L232
Added lines #L223 - L232 were not covered by tests
[warning] 235-240: app/oracle/client.go#L235-L240
Added lines #L235 - L240 were not covered by tests
[warning] 242-242: app/oracle/client.go#L242
Added line #L242 was not covered by tests
conn, err = slinkygrpc.NewClient(c.addr, opts...) | ||
|
||
// attempt to connect + wait for change in connection state | ||
if c.blockingDial { | ||
// connect | ||
conn.Connect() | ||
|
||
if err == nil { | ||
conn.WaitForStateChange(ctx, connectivity.Ready) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add tests for the createConnection
method.
The changes to the createConnection
method are not covered by tests. Ensure that unit tests are added to verify its functionality.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 185-194: app/oracle/client.go#L185-L194
Added lines #L185 - L194 were not covered by tests
func (c *GRPCClient) Version(ctx context.Context, req *oracleservertypes.QueryVersionRequest, _ ...grpc.CallOption) (res *oracleservertypes.QueryVersionResponse, err error) { | ||
c.mutex.Lock() | ||
defer c.mutex.Unlock() | ||
|
||
start := time.Now() | ||
defer func() { | ||
// Observe the duration of the call as well as the error. | ||
c.metrics.ObserveOracleResponseLatency(time.Since(start)) | ||
c.metrics.AddOracleResponse(metrics.StatusFromError(err)) | ||
}() | ||
|
||
// set deadline on the context | ||
ctx, cancel := context.WithTimeout(ctx, c.timeout) | ||
defer cancel() | ||
|
||
if c.client == nil { | ||
return nil, fmt.Errorf("oracle client not started") | ||
} | ||
|
||
return c.client.Version(ctx, req, grpc.WaitForReady(true)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add tests for the Version
method.
The Version
method is not covered by tests. Ensure that unit tests are added to verify its functionality.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 246-255: app/oracle/client.go#L246-L255
Added lines #L246 - L255 were not covered by tests
[warning] 258-263: app/oracle/client.go#L258-L263
Added lines #L258 - L263 were not covered by tests
[warning] 265-265: app/oracle/client.go#L265
Added line #L265 was not covered by tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...