refactor: ibc port router for app callbacks #6314
Draft
Conversation
This file contains 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
|
Important Review SkippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 (
|
damiannolan
commented
May 15, 2024
9 tasks
damiannolan
commented
May 16, 2024
colin-axner
reviewed
May 16, 2024
Comment on lines
+90
to
+91
| timeoutHeight clienttypes.Height, | ||
| timeoutTimestamp uint64, |
There was a problem hiding this comment.
we should remove timeout info from the interface. Left for now since it breaks callbacks interface, but I don't see a reason to leave it for callbacks
colin-axner
reviewed
May 16, 2024
| } | ||
|
|
||
| // TODO: the packet sender is now passed in, it is possible to remove `GetPacketSender` in favour of this arg |
There was a problem hiding this comment.
we can remove the GetPacketSender interface now! Wahoo!
|
The ics20 code needs to be taken care of and some tests refactored. Just did what was needed to resolve the merge conflicts and get most tests passing. |
colin-axner
reviewed
Aug 1, 2024
Comment on lines
101
to
105
| for _, prefix := range k.Keys() { | ||
| if strings.Contains(module, prefix) { | ||
| return k.AppRouter.GetRoute(prefix) | ||
| } | ||
| } |
There was a problem hiding this comment.
lets make this well documented since the implications are subtle cc @damiannolan
* not working yet * More tests fixed. * added message error * commented out tests * Wire approuter to e2e testing simapp * Delete obsolete tests * remove possibly unneeded call * Removed tests no longer needed * test: fix commented/removed tests --------- Co-authored-by: Colin Axnér <[email protected]>
#7010) * chore: adding updates to composite router and scaffolding for IBCLegacyModule * chore: support construction of LegacyIBCModules over consecutive calls to AddRoute * chore: removed ack handhsake impl * chore: fix linter * chore: lint * Linter * chore: addressing PR feedback * chore: routeToLegacyModule returns a single item * Update modules/core/05-port/types/router_v2.go * Update modules/core/05-port/types/router_v2.go * Update modules/core/05-port/types/router_v2.go * chore: fix errors from merge conflict --------- Co-authored-by: bznein <[email protected]> Co-authored-by: Colin Axnér <[email protected]>
* chore: adding updates to composite router and scaffolding for IBCLegacyModule * chore: support construction of LegacyIBCModules over consecutive calls to AddRoute * chore: removed ack handhsake impl * chore: fix linter * chore: lint * Linter * chore: addressing PR feedback * chore: rename IBCModule to ClassicIBCModule and add new IBCModule interface * fix: e2e for fee wrapped channels --------- Co-authored-by: bznein <[email protected]> Co-authored-by: Colin Axnér <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+70
to
+74
| for prefix := range rtr.routes { | ||
| if strings.Contains(module, prefix) { | ||
| return rtr.routes[prefix], true | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
Comment on lines
+109
to
+113
| for prefix := range rtr.legacyRoutes { | ||
| if strings.Contains(module, prefix) { | ||
| return rtr.legacyRoutes[prefix], true | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
* chore: adding wrapping functions to ics29 * chore: added docstring to VersionWrapper interface * chore: fix linter * refactor: use unsafe unwrap func in OnChanOpenInit * chore: fixing wiring in app.go and fee tests * fix: fee middleware tests * fix: ics27 controller tests * wip: debugging callbacks * chore: fix linter * chore: fix miswiring in callbacks module * chore: modifications to binary simapp * chore: do not unwrap a version if it is an empty string * Update modules/apps/27-interchain-accounts/controller/ibc_middleware.go * Apply suggestions from code review * chore: add more code docs --------- Co-authored-by: Colin Axnér <[email protected]>
* chore: add fee implementation for OnChanOpenTry * chore: fix wiring in app.gos * chore: updated callbacks tests to handle new OnChanOpenTry * chore: lint, correct error string * chore: remove commented code * chore: addressing PR feedback
* refactor: use new port router for OnChanOpenAck * fix: transfer test * fix: controller tests * fix: legacy ibc module handler on disabling
* chore: add fee implementation for OnChanOpenTry * chore: fix wiring in app.gos * chore: updated callbacks tests to handle new OnChanOpenTry * chore: lint, correct error string * chore: remove commented code * chore: implementing OnChanUpgradeInit * chore: linter * chore: removed redundant test * chore: addressing PR feedback
* deps: bump cosmos-sdk to v0.50.9 (#6828) * deps: bump cosmos-sdk to v0.50.8 * chore: update changelog * deps: bump cosmossdk.io/client to v2.0.0-beta.3. bump x/upgrade to v0.1.4 * chore: make tidy-all * test: bump to 3f6796fba413cca for testing purposes. * deps: bump cosmos sdk to 0.50.9 * Update CHANGELOG.md * chore: update CHANGELOG for submodules. --------- Co-authored-by: DimitrisJim <[email protected]> * chore(api!): move checks from Transfer to OnSendPacket * Fix merge error * fix to test * Add Require() back * Update modules/apps/transfer/ibc_module.go Co-authored-by: colin axnér <[email protected]> * Linter * Removed wrong merge results * reintroduce test * Move check for isSendEnabledCoins to keeper * new tests * fixed test * linter * Removed checks and added comment * removed extra parentheses * Revert "Merge remote-tracking branch 'origin' into bznein/6949/msgTransferWrapperToSendPacket" This reverts commit be9f558, reversing changes made to 36e4b05. * Delete commented out code --------- Co-authored-by: Damian Nolan <[email protected]> Co-authored-by: DimitrisJim <[email protected]> Co-authored-by: colin axnér <[email protected]>
* chore: implement OnChanUpgradeTry * wip: partial implementation for OnChanUpgradeAck * chore: updated tests to work with new OnChanUpgradeAck implementation
* chore: implement OnChanUpgradeTry * wip: partial implementation for OnChanUpgradeAck * chore: updated tests to work with new OnChanUpgradeAck implementation * chore: implemented OnChanUpgradeOpen * chore: addressing PR feedback
) * refactor: update OnRecvPacket to use result type in favour of ack interface * refactor: update recv packet handling in interchain accounts, update event func arg to use concrete type * api! remove Acknowledgement interface from exported pkg * refactor: modifications to test code for api removal * lint: fix duplicate imports * refactor: use concrete acknowledgement in callbacks test * doc: update inline godocs for OnRecvPacket in ibccallbacks * fix: remove wrapping of result ack in callbacks recv packet * test: refactor writeAck tests to use expected error format * chore: add godocs and stringer impl for new recv result types * chore: add issue link for async acknowledgements in 29-fee * doc: update godocs for RecvPacketResult and status enums * chore: address pr comments, test updates and inline docs
* test not working * refactored test and removed unneeded caese
* refactor: use new port router for OnAcknowledgementPacket * fix: msg server update * fix: ics27 host tests * fix: ics27 controller tests * lint * chore: docs * add type assertions * add 29 type assertion
* initial commit * revert change * loop callbacks * fix loop * wip * chore: fixing remainder of timeout test failures * chore: check type assertion * PR feedbacK --------- Co-authored-by: chatton <[email protected]>
…7142) * chore: updated handlers to use reverseCallbacks function * chore: add reconstructVersion to im * chore: remove duplicate import * chore: pr feedback
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.
Description
closes: #6941
closes: #3371
closes: #4427
closes: #4444
closes: #5774
closes: #5814
closes: #71
closes: #3312
closes: #3817
closes: #3727
closes: #41
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.