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.

Sign up for GitHub

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

Jump to bottom

ICS29: Add fee transfer command #2682

Merged
merged 13 commits into from
Oct 10, 2022
Merged

ICS29: Add fee transfer command #2682

merged 13 commits into from
Oct 10, 2022

Conversation

romac
Copy link
Member

@romac romac commented Sep 27, 2022 •
edited by ljoss17
Loading

Closes: #2714

See #1981 (comment)

Description

Add a fee transfer command which can be used to exercise the relayer incentivization support introduced in #1981.

Testing

In order to test the fee transfer command, open 3 terminals:

First terminal

Setup the test chains

  1. Run nix shell github:informalsystems/cosmos.nix#ibc-go-v5-simapp or nix shell --extra-experimental-features nix-command --extra-experimental-features flakes github:informalsystems/cosmos.nix#ibc-go-v5-simapp
  2. cd to the hermes repo
  3. Start the test chains with CHAIN_COMMAND_PATH=simd cargo run --bin test_setup_with_fee_enabled_binary_channel

Second terminal

Start a Hermes instance

  1. Run nix shell github:informalsystems/cosmos.nix#ibc-go-v5-simapp or nix shell --extra-experimental-features nix-command --extra-experimental-features flakes github:informalsystems/cosmos.nix#ibc-go-v5-simapp
  2. cd to the hermes repo
  3. Run source data/test/binary-channels.env
  4. Update max_msg_num = 30 to max_msg_num = 4 in the file relayer-config.toml located in data/test/
  5. Run CHAIN_COMMAND_PATH=simd cargo run --bin hermes -- --config $RELAYER_CONFIG start

Third terminal

Transfer some tokens with fees. In addition this tests that if the pay message is not in the same batch as the transfer message, the relayer still receives the fees

  1. cd to the hermes repo
  2. Run source data/test/binary-channels.env
  3. Check the balance of the relayers and users, in this case the only ones which will change are the relayer on chain A, user1 on chain A and user2 on chain B:
  • gaiad --node $NODE_A_RPC_ADDR query bank balances $NODE_A_WALLETS_RELAYER_ADDRESS
  • gaiad --node $NODE_A_RPC_ADDR query bank balances $NODE_A_WALLETS_USER1_ADDRESS
  • gaiad --node $NODE_B_RPC_ADDR query bank balances $NODE_B_WALLETS_USER2_ADDRESS
  1. Transfer 30 times 5321 samoleans from user1 to user2:
  • cargo run --bin hermes -- --config $RELAYER_CONFIG fee transfer --dst-chain $CHAIN_ID_B --src-chain $CHAIN_ID_A --src-port $PORT_A --src-channel $CHANNEL_ID_A --amount 5321 --denom $NODE_A_DENOM --receive-fee 1233 --ack-fee 772 --timeout-fee 2001 --timeout-seconds 120 --recipient $NODE_B_WALLETS_USER2_ADDRESS --key-name user1 --number-msgs 30
  1. Check the balances:
  • The relayer on chain A should have received 30 times 2005 samoleans (60150)
  • user1 on chain A should have lost 30 times 2005+5321 samoleans (219780)
  • user2 on chain B should have received 30 times 5321 ibc/<hash> (159630)

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/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

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

@romac romac changed the title Add fee transfer command ICS29: Add fee transfer command Sep 27, 2022
@romac romac mentioned this pull request Sep 27, 2022
Merged
8 tasks
@ljoss17 ljoss17 marked this pull request as ready for review October 7, 2022 11:22
seanchen1991
seanchen1991 approved these changes Oct 7, 2022
View reviewed changes
Copy link
Contributor

@seanchen1991 seanchen1991 left a comment

Choose a reason for hiding this comment

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

I love the effort being put into comments and documentation 😄

@romac
Copy link
Member Author

romac commented Oct 7, 2022

love the effort being put into comments and documentation 😄

Yep, thanks @ljoss17 for the thorough testing instructions :)

romac
romac commented Oct 10, 2022
View reviewed changes
scripts/auto_gen_templates.sh Outdated Show resolved Hide resolved
scripts/auto_gen_templates.sh Show resolved Hide resolved
@ljoss17 ljoss17 merged commit 4878f8b into soares/ics29-fee-middleware Oct 10, 2022
@ljoss17 ljoss17 deleted the romac/fee-transfer-cmd branch October 10, 2022 08:43
romac added a commit that referenced this pull request Oct 24, 2022
@soareschen @adizere
@romac @seanchen1991 @ljoss17
ICS29 Fee Middleware Support (#1981)
2122813 
* Add simple_send_tx

Introduce TxConfig

Add test constructor for TxConfig

Add send_tx method on ChainDriver

Accept Wallet instead of WalletAddress for transferring tokens

Implement ibc_token_transfer

Simplify ibc_token_transfer

Token transfer using ibc_token_transfer is now working

* Move AddressType into TxConfig, as it is configured per chain

* Disable GRPC Web and not rely on chain version

* Clean up ibc_transfer_token interface on ChainDriver

* Use longer IBC transfer timeout

* Further increase IBC transfer timeout

* Fix client expiration test in gaia7

* Move tx_raw_ft_transfer to simulation test

* Update protobuf and add basic channel with fee test

* Basic transfer with pay packet is working

* Modularize ibc_token_transfer

* Basic relayer fee payment working

* Implementing registering counterparty address

* Fix fee calculation with register counterparty address working

* Wait for TX confirmation straight from TxResponse

* Introduce Token type to encapsulate Denom and amount

* Set full node config mode to "validator"

Set minimum price on node bootstrap

* Refactor wait_tx to work with all responses

* Add fee methods to ChainDriver

* Add back test with no forward relayer setup

* Add timeout fee test

* Simplify wait control flow using iterator and streams

* Implement pay packet fee async

* Relaying of pay packet fee async is somehow not working

* Reproduce execute_schedule slowdown

* Add test to reproduce clear packet failure

* Apply dirty hack fix

* Fix merge error

* Pay packet fee async test is now working

* Refactor ChainDriver methods into separate extension traits

* Refactor chain bootstrap CLI functions

* Move query CLI functions

* Refactor all remaining CLI methods

* Refactor ChainDriver bootstrap methods into extension trait

* Refactor packet workers to simplify control flow

* Provide latest height for clear_packet_on_start_worker

* Still trying to fix Python test

* Clear on start can only be performed within packet cmd worker

* Fix merge conflict

* Clean up for review

* Refactor common code from spawn_packet_worker to handle_execute_schedule

* Better handle packet cmd worker retry

* Remove worker_subborn_strategy

* Add query counterparty address method

* Add basic test and implementation for query incenvized packets

* Copy fee implementations to ibc-relayer crate

* Copy proto message builders to ibc crate

* Remove fee implementation in test framework and import from ibc-relayer

* Catch panics in hang_on_error

* Fix merge error

* Implement maybe_register_counterparty_address

* Add maybe_register_counterparty_address ChainHandle method

* Fix import error

* Add Link option for auto registering counterparty address

* Add auto forward relayer test

* Add test for using fee feature with non-fee channels

* Update protobuf with new port_id field in MsgRegisterCounterpartyAddress

* Fix cargo doc

* Add RawCoin type with string as denom

* Use Amount instead of u128 in Token

* Turn Token into type alias for Coin<Denom>

* Refactor use of encode_message

* Refactor Coin and Amount definitions into separate modules

* Add parsing logic for incentivized packet event

* Update protobuf

* Update CounterpartyAddress to CounterpartyPayee

* Add IncentivizedPacket event to IbcEvent

* Fix itertools dependency

* Testing of incentivized packet event is working

* Fix merge error

* Split fee tests into separate files

* Fix fee tests not running on CI, and fix test_auto_forward_relayer

* Test incentivized packet event after pay packet fee async

* Fix merge error

* Update ibc-go

* Update Cosmos.nix and ibc-go version

* Update Cosmos.nix

* Fix merge error

* Use back Cosmos.nix master

* Enable general CI test for ibc-go-v4-simapp

* Add register payee test

* Initial draft implementation for register-payee CLI

* Add register counterparty payee CLI

* Fix clippy

* Add test for registering invalid counterparty payee

* Add example config for auto_register_counterparty_payee

* Fix clippy

* Fix test_pay_packet_fee_async error caused by merge conflict

* Fix clippy

* Refactor coin and amount constructs into separate modules

* Follow the same coin display format as in ibc-go

* Initial token amount cannot exceed u64::MAX

* Small cleanup of fee commands

* Apply suggestions from code review

Co-authored-by: Sean Chen <[email protected]>
Signed-off-by: Soares Chen <[email protected]>

* Update compatibility bound for ibc-go

* Fix no-std check

* ICS29: Add `fee transfer` command (#2682)

* WIP: Add `fee transfer` command

* Added config override in order to change  used to transfer tokens with fee

* Added  unit tests for command argument parsing

* Updated  command in order to send a pay message for each transfer message

* Cargo fmt and cargo clippy fixes

* Use main branch of `ibc-proto`

* Update guide templates and fix script

* Added changelog entry

* Added comments in the body of the 'check_can_send_on_channel' function

* Moved imports to top level in 'cli_utils.rs'

* Removed branch specification for ibc-proto repo in no-std-check Cargo.toml

* Added warning when not GNU sed is used

Co-authored-by: Luca Joss <[email protected]>

* Added 'value_name', 'visible_alias' and unit-tests to the two new commands 'register-counterparty-payee' and 'register-payee'

* Missed renaming some 'ibc' to 'ibc_relayer_types'

* Regenerated Cargo.lock for no-std-check

* Updated guide

* Run more CI integration tests for fee

* Removed ICS29 fee tests using gaia-main, as it has not yet activated the ICS29 feature

* Update tools/integration-test/src/tests/fee/register_payee.rs

Signed-off-by: Sean Chen <[email protected]>

* Update tools/integration-test/src/tests/fee/pay_fee_async.rs

Signed-off-by: Sean Chen <[email protected]>

* Update tools/integration-test/src/bin/test_setup_with_fee_enabled_binary_channel.rs

Signed-off-by: Sean Chen <[email protected]>

* Update tools/integration-test/src/bin/test_setup_with_fee_enabled_binary_channel.rs

Signed-off-by: Sean Chen <[email protected]>

* Update tools/integration-test/src/tests/fee/no_forward_relayer.rs

Signed-off-by: Sean Chen <[email protected]>

* Update tools/integration-test/src/tests/fee/timeout_fee.rs

Signed-off-by: Sean Chen <[email protected]>

* Update tools/integration-test/src/tests/fee/auto_forward_relayer.rs

Signed-off-by: Sean Chen <[email protected]>

* Update tools/integration-test/src/bin/test_setup_with_fee_enabled_binary_channel.rs

Signed-off-by: Sean Chen <[email protected]>

* Formatting

* Increment `maybe_register_counterparty_payee` metric

Co-authored-by: Luca Joss <[email protected]>
Signed-off-by: Romain Ruetschi <[email protected]>

Signed-off-by: Soares Chen <[email protected]>
Signed-off-by: Sean Chen <[email protected]>
Signed-off-by: Romain Ruetschi <[email protected]>
Co-authored-by: Adi Seredinschi <[email protected]>
Co-authored-by: Romain Ruetschi <[email protected]>
Co-authored-by: Sean Chen <[email protected]>
Co-authored-by: Luca Joss <[email protected]>
Co-authored-by: Luca Joss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanchen1991 seanchen1991 seanchen1991 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@romac @seanchen1991 @ljoss17