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 support for wildcards in packet filter #1931

Merged
merged 70 commits into from
Mar 22, 2022

Conversation

hu55a1n1
Copy link
Member

@hu55a1n1 hu55a1n1 commented Mar 2, 2022

Closes: #1927

For testing, follow the instructions at https://github.com/cosmos/interchain-accounts-demo or look at the integration test.

For example, to only relay on channel-0 and on all ICA channels:

[chains.packet_filter]
policy = 'allow'
list = [
  ['ica*', '*'],    # <- wildcards
  ['transfer', 'channel-0'],
]

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

relayer/src/config.rs Outdated Show resolved Hide resolved
relayer/src/config.rs Outdated Show resolved Hide resolved
relayer/src/supervisor/scan.rs Outdated Show resolved Hide resolved
@romac
Copy link
Member

romac commented Mar 18, 2022

Added a proper integration test which performs an ICA bank transfer.

.register_interchain_account(&wallet_a.address(), &connection_a.connection_id_a.as_ref())?;

let channel_id_a: TaggedChannelId<ChainA, ChainB> =
TaggedChannelId::new("channel-0".parse().unwrap());
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way we can get back the channel ID that is not hard coded? Should Hermes also implement CLI and function for registering interchain account instead of doing it through Gaia?

Copy link
Member

Choose a reason for hiding this comment

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

Is there any way we can get back the channel ID that is not hard coded?

Maybe we can query the channels for a given connection?

Copy link
Member

Choose a reason for hiding this comment

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

Should Hermes also implement CLI and function for registering interchain account instead of doing it through Gaia?

I don't think that should be the responsibility of the relayer, though it might be useful for testing but it's not clear whether it's worth the cost or not at the moment. Maybe if we hear the need from operators.

We can discuss during the weekly sync, but let's keep it out of this PR anyway.

tools/test-framework/src/chain/driver.rs Outdated Show resolved Hide resolved
tools/test-framework/src/chain/driver.rs Outdated Show resolved Hide resolved
.github/workflows/integration.yaml Show resolved Hide resolved
tools/integration-test/src/tests/mod.rs Outdated Show resolved Hide resolved
tools/integration-test/src/tests/ica_filter.rs Outdated Show resolved Hide resolved
tools/integration-test/src/tests/ica_filter.rs Outdated Show resolved Hide resolved
@ancazamfir
Copy link
Collaborator

Great stuff! Looks good to me. One unrelated question but showed up while testing this. Why do we spawn the packet workers on OpenAckChannel and OpenConfirmChannel events? The following sequences result in different worker set for hermes:

  • hermes start then hermes create channel...
  • hermes create channel... then hermes start

Code is here https://github.com/informalsystems/ibc-rs/blob/1481a16ee077f6708e3f2ce1866f8e90340fdf46/relayer/src/supervisor.rs#L403-L405
and here
https://github.com/informalsystems/ibc-rs/blob/1481a16ee077f6708e3f2ce1866f8e90340fdf46/relayer/src/supervisor.rs#L419-L421

@romac romac merged commit bc989c3 into master Mar 22, 2022
@romac romac deleted the hu55a1n1/1927-packet-filtering-wildcards branch March 22, 2022 14:41
hu55a1n1 added a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Add filter match support

* Implement AsRef<str> for PortId and ChannelId

* Fix Deserialize validation for filters

* Implement regex support

* Add FIXME

* Fix clippy errors

* Fix regex validation

* Refactoring and renaming

* Update config.toml

* Add unclog entry

* Add documentation for `PacketFilter` and associated types.

* Fix deserialize impl for FilterPatterns

* Rename filter_match.rs to filter_pattern.rs

* Add `FromStr` and remove `Into<String>` for `ics04_channel::Version`

* Parse channel `Version` directly in `create channel` command

* Use `Version` instead of `String` in `Channel::new`

* Add `ChannelVersionOverride` trait to override the channel version in integration tests

* Fix warning

* WIP: Write an integration test that creates a ICA channel

* Add facility to modify genesis file

* Rename some things

* Add packet filtering snippet to relayer config example.

* Add test for deserializing packet filter policy

* Add packet filter policy serialization test.

* Fix serialize_packet_filter_policy test

* Cleanup config tests

* Fix TOML serialization of ChannelFilters and FilterPattern

* Rename some methods

* Move filter-related code into its own module

* Uncomment packet filter policy in relayer example config

* Fix link in doc comment

* Remove some unnecessary clones

* Add test for iter_exact and impl PartialEq for Wildcard

* Get iter_exact test passing

* Add allow and deny filter tests

* Disable ICA filter test

* Rename `spec` to `filters`

* Update `regex` crate to v1.5.5

* Update lockfiles

* Revert "Add facility to modify genesis file"

This reverts commit dd803a9.

* Comment out ICA filter test

* Add `ChannelFilters::new`

* Add working ICA filter integration test

* Add test for when ICA channels are disallowed

* Apply the channel filter to channel handshakes as well

* Enable ICA filter test on CI

* Update Nix path to icad

* Refactor ICA test

* Add changelog entry

* Extend ICA filter test with ICA transfer

* Add facility for modifying the genesis file

* Modify genesis file to allow MsgSend messages over ICA

* Cleanup

* Add comments

* Re-enable ordered channel test

* Remove top-level unrelated file

* Use `CHAIN_COMMAND_PATH` env variable to run the test with icad instead of gaiad without having to hard code the path

* Move ICA driver methods into their own module as functions

* Change signature of `assert_eventual_wallet_amount` to take wallet address

* Add some doc comments

* Rename `ica-filter` test feature and module to `ica`

* Remove duplicate changelog entry

* Add back missing file

Co-authored-by: Sean Chen <[email protected]>
Co-authored-by: Romain Ruetschi <[email protected]>
Co-authored-by: Soares Chen <[email protected]>
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.

Support for Interchain accounts filtering policy
5 participants