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 e2e test for ics27 host params #2118

Closed
3 tasks
colin-axner opened this issue Aug 25, 2022 · 7 comments · Fixed by #6067
Closed
3 tasks

Add e2e test for ics27 host params #2118

colin-axner opened this issue Aug 25, 2022 · 7 comments · Fixed by #6067
Assignees
Labels
e2e help wanted Issues for which we would appreciate help/support from the community

Comments

@colin-axner
Copy link
Contributor

Summary

Add an e2e test for modifying ics27 host params. Should ensure the host is enabled and packets can be received. Should ensure only allowed messages can be executed and non-allowed messages cannot be executed. Once the host is disabled, packets should not be capable of being received.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@crodriguezvega
Copy link
Contributor

Use gov v1 message when #3520 is merged.

@crodriguezvega crodriguezvega added the help wanted Issues for which we would appreciate help/support from the community label Jan 16, 2024
@crodriguezvega crodriguezvega moved this to Backlog in ibc-go Jan 16, 2024
@gjermundgaraba
Copy link
Contributor

gjermundgaraba commented Mar 23, 2024

I would like to take this one on.

It seems like some of the test is already present (

func (s *InterchainAccountsParamsTestSuite) TestHostEnabledParam() {
), but it is missing testing that the disabled host is actually effective (packets do not flow), so the plan I have is to extend the existing TestHostEnabledParam test:

  1. Ensure the host is enabled (this already exists in the test)
  2. Ensure it works to create interchain account before disabling (this is tested elsewhere obviously, but this would be for the logical flow of proof that disabling the host works is the reason for the change in effect. I also see this this approach done other places in the e2e tests, but happy to skip this step if you think it is unnecessary)
  3. Disable the host (this already exist in the test)
  4. Ensure the host is disabled (this already exists in the test)
  5. Ensure that it does not work to create a new interchain account

@crodriguezvega
Copy link
Contributor

That would be fantastic; Thank you, @gjermundgaraba!

The steps you propose sound fine to me. I think I would suggest in step 5 to try to send a message to the host instead of trying to create a new interchain account. That should also exercise in OnRecvPacket the check we are interested in. Afterwards you can verify that the message did not execute (e.g. funds were not transferred). And if you want to go the extra mile, you could check that the error in the acknowledgement is what it should be supposed to be (for this you can take as inspiration this code by courtesy of @srdtrk in the PR to add queries to ICA, where the acknowledgement is retrieved from the events).

If you try to register a new interchain account, the test could fail here, unless you register the interchain account with a different owner address. Alternatively you could also just skip step 2 and then try to register the interchain account after disabling host functionality.

I think both test flows would work, so I leave it up to you to decide which one you prefer. :) If you have any questions, let us know!

@gjermundgaraba
Copy link
Contributor

I've got a solution going in a branch, but I am hesitant to pollute the PR list until the PR you mentioned has been merged because I had to copy some of the utility code to do fetch the event and attribute (QueryTxsByEvents).

I can just wait until that has been merged if that is coming in soon. If not, I could also open the PR as is, and if that would get merged first I can open a PR to fix the conflict that would appear in the aforementioned PR.

gjermundgaraba@38aa8a5

@crodriguezvega
Copy link
Contributor

I've got a solution going in a branch, but I am hesitant to pollute the PR list until the PR you mentioned has been merged because I had to copy some of the utility code to do fetch the event and attribute (QueryTxsByEvents).

I can just wait until that has been merged if that is coming in soon. If not, I could also open the PR as is, and if that would get merged first I can open a PR to fix the conflict that would appear in the aforementioned PR.

gjermundgaraba@38aa8a5

Awesome! We should be merging #5785 this week, so I will comment here once that's done and you can open the PR. Thanks for the quick implementation!

@DimitrisJim
Copy link
Contributor

Hope you've gotten the e2e running locally to at least check things out! This is tricky first time around but the README.md in the e2e folder should have most of what you need.

@gjermundgaraba
Copy link
Contributor

Hope you've gotten the e2e running locally to at least check things out! This is tricky first time around but the README.md in the e2e folder should have most of what you need.

Yes, the README was pretty easy to understand! I've managed to get them running both from the Makefile and directly in the IDE for debugging.

@github-project-automation github-project-automation bot moved this from Backlog 🕐 to Done 🥳 in ibc-go Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e help wanted Issues for which we would appreciate help/support from the community
Projects
Status: Done 🥳
Development

Successfully merging a pull request may close this issue.

4 participants