-
Notifications
You must be signed in to change notification settings - Fork 607
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
Adding ICA Host module #1564
Adding ICA Host module #1564
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
app/upgrades/v9/upgrades.go
Outdated
// create ICS27 Host submodule params | ||
hostParams := icahosttypes.Params{ | ||
HostEnabled: true, | ||
AllowMessages: []string{"/cosmos.bank.v1beta1.MsgSend"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be safe here, what we can/should do, is the use the types directly.
i.e.
AllowMessages: []string{
sdk.MsgTypeURL(&banktypes.MsgSend{}),
// ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
such sad that we have to do this :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to add send, gov vote, stake, reward claim, and gamm messages.
I think we just go change the IBC interface to make it more sane to include everything by default >:( (Aditya mentioned this is on their roadmap, may be we can just help with a PR here, if they'd be down to release sooner)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And Authz, they can take the L of executing two messages via authz'ing to themself lol
We should use this PR as a reference for more boilerplate we need to eliminate |
Codecov Report
@@ Coverage Diff @@
## main #1564 +/- ##
==========================================
+ Coverage 19.49% 19.57% +0.08%
==========================================
Files 243 243
Lines 32266 32297 +31
==========================================
+ Hits 6290 6322 +32
+ Misses 24822 24821 -1
Partials 1154 1154
Continue to review full report at Codecov.
|
(cherry picked from commit 09f3184) # Conflicts: # app/upgrades/v9/constants.go # app/upgrades/v9/upgrades.go
* Adding ICA Host module (#1564) (cherry picked from commit 09f3184) # Conflicts: # app/upgrades/v9/constants.go # app/upgrades/v9/upgrades.go * fix conflicts Co-authored-by: Dev Ojha <[email protected]> Co-authored-by: Roman <[email protected]>
Closes: #XXX
What is the purpose of the change
Adds Interchain account host module
Brief Changelog
Testing and Verifying
References I'm using in writing this:
As long as we can verify that IBC channel creation and sends still work post this getting merged, I think were fine.
Documentation and Release Note
Unreleased
section inCHANGELOG.md
? not yet