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

Add interchain account address to packet 1 acknowledgement #333

Closed

Conversation

colin-axner
Copy link
Contributor

Description

closes: #318


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 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.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

add interchain account address to acknowledgement for packet 1
add placeholder code for onAck parsing of ack
start writing tests
@colin-axner colin-axner changed the title feat parse interchain acc address on packet 1 Add interchain account address to packet 1 acknowledgement Aug 13, 2021
Comment on lines +253 to +256
if packet.Sequence == 1 && ack.Success() {
// pass interchain account address into base applications
// connected to the middleware
}
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 information shouldn't be set in state since we could have domain conflicts (this module acts as a host and controller in some cases). It's usefulness is passing this information to helper modules, but even so we will likely end up using the version to convey this information

@colin-axner colin-axner marked this pull request as ready for review August 27, 2021 10:56
@colin-axner colin-axner marked this pull request as draft August 27, 2021 10:56
@colin-axner
Copy link
Contributor Author

Sweet, I finally got the codeowner file to work right

@seantking
Copy link
Contributor

Sweet, I finally got the codeowner file to work right

Nice!!

Copy link
Contributor

@seantking seantking left a comment

Choose a reason for hiding this comment

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

I'm wondering if we should even go down this path at the moment.

I think it's not going to be a large amount of work to get the interchain-account address set on the controller side via the version string. It's basically just the query endpoint on the ibc-go side and then a minor change on the hermes side (you had some ideas for a more complete approach on the hermes side, but a short term solution is to just replace a hardcoded value with a call to the query endpoint on our side).

I think we could potentially get this done pretty quickly, and it'll give us the nicest UX and feature completeness within interchain-accounts.

@colin-axner
Copy link
Contributor Author

I'm wondering if we should even go down this path at the moment.

Yea I agree. I'd prefer to just move forward with the version solution. Shall we close this pr?

@seantking
Copy link
Contributor

I'm wondering if we should even go down this path at the moment.

Yea I agree. I'd prefer to just move forward with the version solution. Shall we close this pr?

Yep, yep. I'll update the issue as well. Thank you for starting this 🤝

@seantking seantking closed this Sep 3, 2021
@colin-axner colin-axner deleted the colin/318-parse-acc-from-ack branch September 3, 2021 08:49
faddat pushed a commit to notional-labs/ibc-go that referenced this pull request Mar 1, 2022
CosmosCar pushed a commit to caelus-labs/ibc-go that referenced this pull request Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants