Allow for non-'static Module
#493
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #493 +/- ##
==========================================
+ Coverage 71.52% 71.59% +0.07%
==========================================
Files 125 125
Lines 15890 15939 +49
==========================================
+ Hits 11365 11412 +47
- Misses 4525 4527 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
I can totally see how this change was like a hassle for you :)
I had thought about the changes in this PR and the previous one in #480. It seems like we might be reducing our support for Tendermint chains and putting more responsibility on implementors to handle things on their end (like with the IbcModuleWrapper trait we defined in basecoin-rs). Maybe we should create a new trait called ModuleSafe that is bound by send + sync + 'static. We can then pass it to the Router as an associated type. (ofc as one possible solution, there might be other ways to handle it) WDYT?
| pub trait Router<'m> { | ||
| /// Returns a reference to a `Module` registered against the specified `ModuleId` | ||
| fn get_route(&self, module_id: &ModuleId) -> Option<&dyn Module>; | ||
| fn get_route(&self, module_id: &ModuleId) -> Option<&(dyn Module + 'm)>; | ||
|
|
||
| /// Returns a mutable reference to a `Module` registered against the specified `ModuleId` | ||
| fn get_route_mut(&mut self, module_id: &ModuleId) -> Option<&mut dyn Module>; | ||
| fn get_route_mut(&mut self, module_id: &ModuleId) -> Option<&mut (dyn Module + 'm)>; |
There was a problem hiding this comment.
Seems like Router is making us to use lifetime everywhere. I think Boxing the return types of get_route and get_route_mut to Option<Box<&dyn Module>> and Option<Box<&mut dyn Module>> respectively, alleviates using lifetime annotations all over the repo.
There was a problem hiding this comment.
Boxing a reference seems odd; I suspect that this doesn't actually solve the problem if you want to allow to return Modules with a non-'static lifetime from get_route. In other words, it is not sufficient to remove the 'static bound from Module, you also have to let them be return-able from the Router; and I'd be surprised that there'd be a way to do that without tracking the lifetimes?
I'll look for a counter-example to better show what I mean
There was a problem hiding this comment.
Actually, the best solution is simpler than my implementation and yours: it's to simply not change anything! Yes, very silly of me... :
pub trait Router {
fn get_route(&self, module_id: &ModuleId) -> Option<&dyn Module>;
}which, without any lifetimes elided, is equivalent to:
pub trait Router {
fn get_route<'s, 'a>(&'s self, module_id: &'a ModuleId) -> Option<&'s (dyn Module + 's)>;
}The crucial element that makes it work is the + 's in dyn Module + 's; that is, by default, dyn Modules have the proper lifetime bound (especially not 'static)!
Why my solution worked
My solution worked because implicitly, 'm < 's ('s outlives 'm). So implementations that return a &'s (dyn Module + 's) satisfy the requirement (probably all real-world implementations).
But really the 'm is unneeded and can simply be removed.
Why your solution worked
Your solution, without lifetime elision, expands to:
pub trait Router {
fn get_route<'s, 'a>(&'s self, module_id: &'a ModuleId) -> Option<&'s Box<dyn Module + 's>>;
}The dyn Modules have a 's lifetime bound, which is what we want; the Box is not necessary and can be removed.
How to fix this
I think the PR should have been as simple as removing the AsAnyMut bound on Module (and fixing the tests), and not touching the Router at all... Yes, very silly. I'll try that on Monday!
There was a problem hiding this comment.
Thanks for looking into it. Glad we can keep things simple 👌
| #[derive(Debug)] | ||
| pub struct DummyTransferModule { | ||
| pub struct DummyTransferContext { | ||
| ibc_store: Arc<Mutex<MockIbcStore>>, | ||
| } |
There was a problem hiding this comment.
I get where you're coming from with differentiating the Module and Context implementation. But, in reality, both DummyTransferModule and DummyTransferContext are pretty much the same, just with different names. Plus, DummyTransferContext uses the same ibc_store that's already provided by MockContext. It can get confusing trying to figure out the functionality of each when you're looking at both MockContext and DummyTransferContext/DummyTransferModule at the same time.
Why not extend the capabilities of MockContext by applying token transfer contexts on it? was there any reservation?
There was a problem hiding this comment.
Yes this is the messiest part of the solution. The core problem is that the previous way it was implemented downcasted the Module to a DummyTransferModule, and we can on longer do that because downcasting requires core::any::Any, which requires 'static. See #491 for how I think this could be improved.
I ended up going for this because it worked. I think there was an issue when implementing on the MockContext, because we still need the traits for the callback handlers... But then again there might be a way to make it work.
Note that I find that the shared store architecture is confusing because it couples our types in confusing ways; the whole thing is in need of a refactor IMO.
|
Closing in favor of #499. |
Closes: #490
Description
This PR removes the
'staticbound from theModuletrait. This has a ripple effect of requiring us to track that lifetime on theRouter,ValidationContext, andExecutionContext.Associated basecoin-rs PR.
PR author checklist:
unclog.docs/).Reviewer checklist:
Files changedin the GitHub PR explorer.