chore(api!): replace Router with PortKeeper in IBCKeeper.#6138
chore(api!): replace Router with PortKeeper in IBCKeeper.#6138chatton merged 12 commits intocosmos:mainfrom
Conversation
bznein
requested review from
AdityaSripal,
charleenfei,
chatton,
colin-axner,
damiannolan and
srdtrk
as code owners
WalkthroughThe update primarily focuses on the removal of the Changes
Assessment against linked issues
Possibly related issues
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 Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // GetRoute() returns a IBCModule for a given module. | ||
| func (k Keeper) GetRoute(module string) (types.IBCModule, bool) { | ||
| return k.Router.GetRoute(module) | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The initialization of the Router field in the Keeper struct is not explicitly shown in the provided output from the NewKeeper function. The function initializes various other keepers and components, but there is no direct reference to the initialization of a Router field. This suggests that either the Router is initialized elsewhere, or it might not be properly initialized, which could lead to a nil pointer dereference when GetRoute() is called.
Given the absence of explicit initialization in the provided constructor and the direct use of Router.GetRoute(module) in the GetRoute() method without any nil checks, there is a potential risk of encountering a nil pointer dereference if Router is not properly initialized elsewhere in the code.
Based on this analysis, the review comment concerning the need to ensure that the Router field is properly initialized before calling GetRoute() is justified. Further investigation into the complete initialization of Router across the codebase would be necessary to fully ensure safety against nil pointer dereferences.
Analysis chain
Ensure that the Router field is properly initialized before calling GetRoute(), as it directly accesses Router which could potentially be nil.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any nil checks or initializations before using Router in GetRoute.
ast-grep --lang go --pattern $'func (k Keeper) GetRoute($_) $_ {
k.Router.GetRoute($_)
}'
Length of output: 92
Script:
#!/bin/bash
# Search for the constructor or initialization method of Keeper where Router might be initialized
ast-grep --lang go --pattern $'func NewKeeper($$$) *Keeper {
$$$
}'
# Verify the actual implementation of GetRoute() to ensure it matches the description and check for nil handling
ast-grep --lang go --pattern $'func (k Keeper) GetRoute($_) $_ {
$$$
}'
Length of output: 3673
There was a problem hiding this comment.
I don't know if this is actually a problem, we could error in the case of the router not being sealed, but at the moment the only usage of the Sealed() fn is to check whether or not we can seal a router a second time.
There was a problem hiding this comment.
@chatton, you're right to consider the implications of the router's state on the system's behavior. The sealing of the router is primarily a safety mechanism to prevent modifications after its initial setup, which is crucial for ensuring that the routing remains consistent and error-free throughout the application's lifecycle.
If the router isn't sealed when GetRoute() is called, and there's a possibility of modification, it could lead to inconsistencies or errors during routing. Implementing error handling or a check within GetRoute() to ensure the router is sealed before proceeding could be a good safeguard. This would help maintain the integrity of the routing process, especially in a dynamic environment where changes to the router might be attempted after initial configuration.
Reviewing the current error handling strategies around the router's usage and considering enhancements could further strengthen the system's robustness against such issues.
There was a problem hiding this comment.
@chatton yeah I'm conflicted :) We could definitely check for k.Router == nil || !k.Router.Sealed() here and either panic or return nil, false in that case. WDYT?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Do we currently panic if the router isn't set on the core keeper? I'm thinking this would be quite the misconfiguration by a chain developer but might be nice to do what we recently did for the queryRouter (c70c9c6) and panic as early as possible as opposed to when a chain launches and does some sort of lookup on it.
Might be overkill though 🤷
There was a problem hiding this comment.
To the rabbit's point. I'd probably want to make the Router private and have an accesor method for it that does the nil/unsealed check, if that was possible. But that's out of scope for this PR. I'd be fine with adding these sanity checks for the time being if others agree.
There was a problem hiding this comment.
yeah I wouldn't be against panicking if the router is not sealed, as it shouldn't be used when it is partially configured.
Normally wouldn't like to panic, but the alternative of false, nil just means a silent failure IMO.
There was a problem hiding this comment.
It would be nice to maybe also make the Router unexported in 05-port.
Aside from that I think there's some other improvements we could probably make to APIs of the core IBCKeeper.
For example right now we require users to create a port router in app.go and bind ibc appmodules there, similarly we retrieve the new light client module router from the clientKeeper and AddRoute for each light client module.
It may be nice to have everything flow through the IBCKeeper, as it essentially is a facade around the set of submodules which make up core ibc.
e.g. IBCKeeper.RegisterClientModule(lightClient) and IBCKeeper.RegisterIBCApplication or something like that, could be useful at some point, maybe these changes may be warranted with depinject work so I think we can hold off on them for now and discuss later. Feel free to drop any thoughts 💭
There was a problem hiding this comment.
definitely sounds nice on first read. Don't see why we should be exposing these routers in app.go instead of keeping them as an impl detail of the core keeper.
chatton
left a comment
chatton
left a comment
There was a problem hiding this comment.
LGTM, left a few small suggestions but nothing major 👍
| // GetRoute() returns a IBCModule for a given module. | ||
| func (k Keeper) GetRoute(module string) (types.IBCModule, bool) { | ||
| return k.Router.GetRoute(module) | ||
| } |
There was a problem hiding this comment.
I don't know if this is actually a problem, we could error in the case of the router not being sealed, but at the moment the only usage of the Sealed() fn is to check whether or not we can seal a router a second time.
Co-authored-by: Cian Hatton <github.qpeyb@simplelogin.fr>
DimitrisJim
left a comment
DimitrisJim
left a comment
There was a problem hiding this comment.
lgtm. Lets wait for another opinion on the rabbit's point though. 🐇
crodriguezvega
left a comment
crodriguezvega
left a comment
There was a problem hiding this comment.
You're on fire, @bznein! 🔥🧯
| ConnectionKeeper *connectionkeeper.Keeper | ||
| ChannelKeeper *channelkeeper.Keeper | ||
| PortKeeper *portkeeper.Keeper | ||
| Router *porttypes.Router |
There was a problem hiding this comment.
Removing this is API breaking, so we should document it in the migration docs of v9. I will also change the PR name to reflect this.
crodriguezvega
changed the title
damiannolan
left a comment
damiannolan
left a comment
There was a problem hiding this comment.
Code changes look great! Thanks for picking this up, just left one comment about aligning APIs as we now have two routers in ibc core - one for ibc apps and one for ibc client modules
5 participants
Description
Removes
RouterfromIBCKeeperand replace calls to it with calls to the new functionPortKeeper.GetRoute()Note: I didn't add tests for this new method for the reason that
GetRouteon the underlying router is already tested, and I wasn't able to get a proper set up for the test. I'm more than open to suggestions!closes: #6133
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit
RouterwithPortKeeperacross modules for improved reliability.GetRoute()function for better route handling in channel operations.