Skip to content
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

Use new port router for onchanupgradetry #7067

Conversation

chatton
Copy link
Contributor

@chatton chatton commented Aug 7, 2024

Description

Marking as R4R for E2Es

closes: #7027

This PR refactors fee middleware, ICA and callbacks to use the new port router.

Some wiring changes in app.gos needed to be fixed also.


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.

  • Targeted PR against the correct branch (see CONTRIBUTING.md).
  • Linked to GitHub issue with discussion and accepted design, OR link to spec that describes this work.
  • Code follows the module structure standards and Go style guide.
  • Wrote unit and integration tests.
  • Updated relevant documentation (docs/).
  • Added relevant godoc comments.
  • Provide a conventional commit message to follow the repository standards.
  • Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
  • Re-reviewed Files changed in the GitHub PR explorer.
  • Review SonarCloud Report in the comment section below once CI passes.

@@ -74,6 +74,50 @@ func (im *LegacyIBCModule) OnChanOpenInit(
return reconstructVersion(im.cbs, negotiatedVersions)
}

// OnChanOpenTry implements the IBCModule interface.
func (im *LegacyIBCModule) OnChanOpenTry(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is almost an exact duplicate, I played around with trying to generalize the Init/Try functions, but it ended up looking even uglier, happy to revisit this idea, but I think it's fine as is for now unless someone has a suggestion for removing the duplication.

@chatton chatton added priority PRs that need prompt reviews feat: port-router labels Aug 7, 2024
Copy link

sonarcloud bot commented Aug 7, 2024

// lower down in the stack. Thus, if it is not a fee version we pass the entire version string onto the underlying
// application.
return im.app.OnChanOpenTry(ctx, order, connectionHops, portID, channelID, counterparty, counterpartyVersion)
if strings.TrimSpace(counterpartyVersion) != "" && counterpartyVersion != types.Version {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if strings.TrimSpace(counterpartyVersion) != "" && counterpartyVersion != types.Version {
if counterpartyVersion != types.Version {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think on Try, the version must be set

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense!

Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, see comment. Quick work and excellently done!

@chatton chatton merged commit 9460400 into feat/port-router Aug 7, 2024
15 of 17 checks passed
@chatton chatton deleted the cian/issue#7027-use-new-port-router-for-onchanupgradetry branch August 7, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: port-router priority PRs that need prompt reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants