move ics4wrapper from keeper to ibc module #3482
Conversation
|
@GNaD13 please provide a commit message for this pr and btw, great work! |
Got it sir |
|
nice :D. after I merged main, it blew up your work a bit I think, sorry |
…dang/remove-ics4wrapper
GNaD13
requested review from
AdityaSripal,
charleenfei,
colin-axner,
damiannolan,
crodriguezvega,
chatton and
DimitrisJim
as code owners
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3482 +/- ##
==========================================
- Coverage 78.84% 78.79% -0.06%
==========================================
Files 187 186 -1
Lines 12948 12943 -5
==========================================
- Hits 10209 10198 -11
- Misses 2311 2317 +6
Partials 428 428
|
There was a problem hiding this comment.
Thank you very much for your contribution, @GNaD13. And sorry for the late response from our side. I left a few comments, but I would like to hear @colin-axner or @damiannolan opinion to confirm if they like this pattern.
| @@ -26,7 +25,6 @@ type Keeper struct { | |||
| cdc codec.BinaryCodec | |||
|
|
|||
| authKeeper types.AccountKeeper | |||
| ics4Wrapper porttypes.ICS4Wrapper | |||
There was a problem hiding this comment.
I think we need to do a similar change for ics 27 controller keeper, right? That's it, we can remove the ics4Wrapper field.
| // ICA Controller keeper | ||
| app.ICAControllerKeeper = icacontrollerkeeper.NewKeeper( | ||
| appCodec, keys[icacontrollertypes.StoreKey], app.GetSubspace(icacontrollertypes.SubModuleName), | ||
| app.IBCFeeKeeper, // use ics29 fee as ics4Wrapper in middleware stack | ||
| IBCFeeMiddleware, // use ics29 fee as ics4Wrapper in middleware stack |
There was a problem hiding this comment.
See my comment above: this should not be needed anymore.
| icaControllerStack = ibcmock.NewIBCModule(&mockModule, ibcmock.NewIBCApp("", scopedICAMockKeeper)) | ||
| app.ICAAuthModule = icaControllerStack.(ibcmock.IBCModule) |
There was a problem hiding this comment.
Why was this changed?
| ibcMockModule := ibcmock.NewIBCModule(&mockModule, ibcmock.NewIBCApp("", scopedICAMockKeeper)) | ||
| app.ICAAuthModule = ibcMockModule | ||
| icaControllerMiddleware := icacontroller.NewIBCMiddleware(ibcMockModule, app.ICAControllerKeeper, app.IBCKeeper.ChannelKeeper) | ||
| icaControllerStack = ibcfee.NewIBCMiddleware(icaControllerMiddleware, app.IBCFeeKeeper, icaControllerMiddleware) |
There was a problem hiding this comment.
We are passing here the icaControllerMiddleware twice, one for app and one for next. Do we like this pattern?
Description
closes: #3371
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
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/) or specification (x/<module>/spec/).godoccomments.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.