chore(cosmosclient): switch to BroadcastModeSync
#2850
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TODO: update BroadcastTx calls so they can actually get the response of the transaction.
Consumer can pass a ctx with a timeout to get the exact same behavior with a better level of customization.
This func will be handy once we'll switch to BroadcastMode Sync.
The test works in BroadcastModeBlock, but not any more in BroadcastTxSync, it will serve as a proof that the transition is OK.
This is already done in `CreateTx` so if it's no longer done in BroadcastTx, this should have very limited impacts. The good part is that removes complexity.
tbruyelle
requested review from
aljo242,
fadeev,
ilgooz,
jeronimoalbi and
lumtis
as code owners
tbruyelle
changed the title
BroadcastModeSync
aljo242
reviewed
Sep 22, 2022
aljo242
previously approved these changes
Sep 23, 2022
jeronimoalbi
previously approved these changes
Sep 27, 2022
jeronimoalbi
previously approved these changes
Sep 27, 2022
aljo242
reviewed
Sep 27, 2022
|
LGTM ! |
Co-authored-by: Alex Johnson <alex@shmeeload.xyz>
Jchicode
pushed a commit
to Jchicode/cli
that referenced
this pull request
Aug 9, 2023
* wip: switch to broadcastMode sync TODO: update BroadcastTx calls so they can actually get the response of the transaction. * refac(cosmosclient): remove timeout param from Wait* funcs Consumer can pass a ctx with a timeout to get the exact same behavior with a better level of customization. * feat: cosmosclient.WaitForTx func This func will be handy once we'll switch to BroadcastMode Sync. * test: add integration test for network publish The test works in BroadcastModeBlock, but not any more in BroadcastTxSync, it will serve as a proof that the transition is OK. * chore: make BroadcastTx works with BroadcastTxSync * test: clean go/bin/appBinary after serve * fix test and remove --broadcastmode flag * when you realize you were requesting the wrong chain... * refac: stop calling faucet on BroadcastTx This is already done in `CreateTx` so if it's no longer done in BroadcastTx, this should have very limited impacts. The good part is that removes complexity. * add CL * test: try with http url * revert changing broadcast mode in chain test * test: ensure the correct spn version is used * test: use env.Ctx() in waitForNextBlock * fix CL * Update changelog.md Co-authored-by: Alex Johnson <alex@shmeeload.xyz> Co-authored-by: Alex Johnson <alex@shmeeload.xyz>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #2721
The main work of this PR was to ensure that
cosmosclient.BroadcastTxcontinues to work as expected, meaning it continues to return the response tx once the tx is confirmed.With the previous setting
BroadcastModeBlockit was easy, but withBroadcastModeSync, you have to wait for the tx to be confirmed, and then reads it data. This is done thanks to a new methodWaitForTxwhich relies onWaitForNextBlockthat was previously added.I also started some integration tests on the
networkcommands, because they are the main consumers ofcosmosclient.BroadcastTx.The flag
--broadcast-modehas been removed from thenodecommand, because it has been added to prevent timeouts whenBroadcastModeBlockwas used. Since this mode is no longer used, I think it's no longer required to expose such setting.