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

docs(CIP-14): define proposed param values #96

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Mar 6, 2024

Currently CIP-14 is ambiguous with respect to the values it proposes for the host module parameters. The table in this PR aims to remove that ambiguity.

The values were chosen based on the latest messages in the forum post. I would appreciate reviews from the CIP authors: @womensrights @asalzmann @sampocs

Note: I think we also need to fill out the Test Cases section of this CIP prior to moving it to review

@jcstein
Copy link
Member

jcstein commented Mar 6, 2024

Per CIP-1, test cases is optional, but for this CIP, I think it is helpful https://github.com/celestiaorg/CIPs/blob/main/cips/cip-template.md#test-cases

| Module.Parameter | Proposed value | Summary | Changeable via Governance |
|-----------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------------------|
| icahost.HostEnabled | `true` | Controls a chains ability to service ICS-27 host specific logic. | False |
| icahost.AllowMessages | `["/ibc.applications.transfer.v1.MsgTransfer", "/cosmos.bank.v1beta1.MsgSend", "/cosmos.staking.v1beta1.MsgDelegate", "/cosmos.staking.v1beta1.MsgBeginRedelegate", "/cosmos.staking.v1beta1.MsgUndelegate", "/cosmos.staking.v1beta1.MsgCancelUnbondingDelegation", "/cosmos.distribution.v1beta1.MsgSetWithdrawAddress", "/cosmos.distribution.v1beta1.MsgWithdrawDelegatorReward", "/cosmos.distribution.v1beta1.MsgFundCommunityPool", "/cosmos.gov.v1.MsgVote"]` | Provides the ability for a chain to limit the types of messages or transactions that hosted interchain accounts are authorized to execute by defining an allowlist using the Protobuf message type URL format. | False |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the table!

False

Should this be False? Potentially there may be desire to add more in the future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I expect more messages to be added to this list in the future but IMO this list should be expanded via the CIP process rather than on-chain governance.

| icahost.HostEnabled | `true` | Controls a chains ability to service ICS-27 host specific logic. | False |
| icahost.AllowMessages | `["/ibc.applications.transfer.v1.MsgTransfer", "/cosmos.bank.v1beta1.MsgSend", "/cosmos.staking.v1beta1.MsgDelegate", "/cosmos.staking.v1beta1.MsgBeginRedelegate", "/cosmos.staking.v1beta1.MsgUndelegate", "/cosmos.staking.v1beta1.MsgCancelUnbondingDelegation", "/cosmos.distribution.v1beta1.MsgSetWithdrawAddress", "/cosmos.distribution.v1beta1.MsgWithdrawDelegatorReward", "/cosmos.distribution.v1beta1.MsgFundCommunityPool", "/cosmos.gov.v1.MsgVote"]` | Provides the ability for a chain to limit the types of messages or transactions that hosted interchain accounts are authorized to execute by defining an allowlist using the Protobuf message type URL format. | False |

### Definitions

- `Host Chain`: The chain where the interchain account is registered. The host chain listens for IBC packets from a controller chain which contain instructions (e.g. cosmos SDK messages) that the interchain account will execute.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just realized a lot of this text is copied verbatim from the ICS27 spec in https://github.com/cosmos/ibc/tree/main/spec/app/ics-027-interchain-accounts . I think its good to have it here but maybe worth noting its a direct copy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can do!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On re-read ICS-27 is linked here so defer to the CIP authors if they want to cite it again.

Copy link
Member

@jcstein jcstein left a comment

Choose a reason for hiding this comment

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

LGTM, but will wait on review from @womensrights @asalzmann and @sampocs

@womensrights
Copy link
Contributor

lgtm, I will submit a seperate pr linking to test cases in the ibc-go repo

Copy link
Contributor

@sampocs sampocs 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 for updating!

@jcstein jcstein merged commit e0275fb into celestiaorg:main Mar 13, 2024
1 check passed
@rootulp rootulp deleted the rp/ica-allowed-messages branch March 13, 2024 17:53
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