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

fix: remove redundant ValidateBasic check in BroadcastTx #357

Conversation

charleenfei
Copy link
Contributor

this check is redundant because ValidateBasic on the messages will be run by the chain binary anyway, running it prematurely leads to strange version issues

context: writing an e2e test for upgrading ibc-go v6 -> v7 runs automatic migrations which bump the client and consensusstate versions of the tendermint client and solo machine, we would need the msg to be broadcasted and validated by the v6 binary rather than the v7 code.

@charleenfei charleenfei requested a review from a team as a code owner December 7, 2022 16:01
@charleenfei
Copy link
Contributor Author

looks like the test failure is coming from an rpc not found in the interchain accounts test, but something went wrong with the docker container not starting :)

@DavidNix
Copy link
Contributor

DavidNix commented Dec 8, 2022

looks like the test failure is coming from an rpc not found in the interchain accounts test, but something went wrong with the docker container not starting :)

I'm re-running the failed one. As common with e2e tests, there's flakiness (especially when dealing with networks).

@charleenfei
Copy link
Contributor Author

charleenfei commented Dec 8, 2022

interesting, it looks like the same test is consistently failing, even on your main branch. it looks like the interchain account creation is failing, which is why the rpc query returns an error.

looks like this error started with this commit: 9359b06

@DavidNix
Copy link
Contributor

DavidNix commented Dec 8, 2022

interesting, it looks like the same test is consistently failing, even on your main branch. it looks like the interchain account creation is failing, which is why the rpc query returns an error.

looks like this error started with this commit: 9359b06

You're right. See #354 (comment)

@charleenfei
Copy link
Contributor Author

@DavidNix is it possible to merge this PR before the failing test is fixed?

@DavidNix
Copy link
Contributor

DavidNix commented Dec 9, 2022

@DavidNix is it possible to merge this PR before the failing test is fixed?

Sure. I normally dislike merging when CI is failing. But this seems like a valid exception.

@DavidNix DavidNix merged commit e8388a0 into strangelove-ventures:main Dec 9, 2022
github-actions bot pushed a commit that referenced this pull request Dec 9, 2022
agouin pushed a commit that referenced this pull request Dec 12, 2022
Co-authored-by: Charly <charly@interchain.berlin>
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.

2 participants