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

change DefaultParams on the host to allow all messages ("*") #2290

Merged

Conversation

georgelombardi97
Copy link
Contributor

Description

Interchain Accounts are used by controller chains to execute transactions on the host chain. My suggestion is just as normal wallets are allowed to submit any transaction, by default, the host should allow all types of messages to be submitted using the ICAs.

Allowing all messages by default makes more sense regarding the purpose of Interchain Accounts. Otherwise, it looks like this parameter should be configured to enable Interchain Accounts.


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

Interchain Accounts are used by controller chains to execute transactions on the host chain. My suggestion is just as normal wallets are allowed to submit any transaction, by default, the host should allow all types of messages to be submitted using the ICAs. 

Allowing all messages by default makes more sense regarding the purpose of Interchain Accounts. 
Otherwise, it looks like this parameter should be configured to enable Interchain Accounts.
@codecov-commenter
Copy link

Codecov Report

Merging #2290 (e7b8e32) into main (9fa6008) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2290      +/-   ##
==========================================
- Coverage   79.52%   79.50%   -0.02%     
==========================================
  Files         175      175              
  Lines       12069    12069              
==========================================
- Hits         9598     9596       -2     
- Misses       2047     2048       +1     
- Partials      424      425       +1     
Impacted Files Coverage Δ
...s/apps/27-interchain-accounts/host/types/params.go 42.10% <100.00%> (+2.63%) ⬆️
...s/apps/27-interchain-accounts/host/keeper/relay.go 71.42% <0.00%> (-3.58%) ⬇️

correct formatting (gofumpt)
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.

I'm in favor of the changes, thanks for the contribution @georgelombardi97! Could you add a changelog entry?

@georgelombardi97
Copy link
Contributor Author

georgelombardi97 commented Sep 21, 2022

I'm in favor of the changes, thanks for the contribution @georgelombardi97! Could you add a changelog entry?
@colin-axner added changelog

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.

Great work, thank you @georgelombardi97!

Copy link
Contributor

@chatton chatton left a comment

Choose a reason for hiding this comment

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

LGTM thanks @georgelombardi97 !

@crodriguezvega crodriguezvega merged commit 17099fe into cosmos:main Sep 21, 2022
mergify bot pushed a commit that referenced this pull request Sep 21, 2022
* change DefaultParams on the host to allow all messages ("*")

Interchain Accounts are used by controller chains to execute transactions on the host chain. My suggestion is just as normal wallets are allowed to submit any transaction, by default, the host should allow all types of messages to be submitted using the ICAs.

Allowing all messages by default makes more sense regarding the purpose of Interchain Accounts.
Otherwise, it looks like this parameter should be configured to enable Interchain Accounts.

* correct formatting (gofumpt)

correct formatting (gofumpt)

* hold wildcard string in a constant

* add changelog

Co-authored-by: george <>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
(cherry picked from commit 17099fe)
crodriguezvega added a commit that referenced this pull request Sep 23, 2022
* update default value allow messages after #2290

* Update parameters.md
mergify bot pushed a commit that referenced this pull request Sep 23, 2022
* update default value allow messages after #2290

* Update parameters.md

(cherry picked from commit 94e61c0)
damiannolan added a commit that referenced this pull request Sep 26, 2022
…2375)

* update default value allow messages after #2290

* Update parameters.md

(cherry picked from commit 94e61c0)

Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
damiannolan added a commit that referenced this pull request Sep 26, 2022
…2356)

* change DefaultParams on the host to allow all messages ("*")

Interchain Accounts are used by controller chains to execute transactions on the host chain. My suggestion is just as normal wallets are allowed to submit any transaction, by default, the host should allow all types of messages to be submitted using the ICAs.

Allowing all messages by default makes more sense regarding the purpose of Interchain Accounts.
Otherwise, it looks like this parameter should be configured to enable Interchain Accounts.

* correct formatting (gofumpt)

correct formatting (gofumpt)

* hold wildcard string in a constant

* add changelog

Co-authored-by: george <>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>
(cherry picked from commit 17099fe)

Co-authored-by: georgelombardi97 <106404363+georgelombardi97@users.noreply.github.com>
Co-authored-by: Carlos Rodriguez <carlos@interchain.io>
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
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.

5 participants