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

Improve DX - Stargate sign and broadcast #1396

Merged
merged 12 commits into from
Jun 19, 2023

Conversation

DavideSegullo
Copy link
Contributor

Hi, as stated in the title of the PR I would like to suggest the inclusion of a new method to sign a tx and then not have to wait for it to be inserted into a block.

Currently it is possible to do this, but by putting workarounds with ts-ignore, I think it is more appropriate to add a method directly.

Why?

In nabla, we are working on a utility to track transactions using websockets, so it is useful to get the hash of the tx right away and then pass it to the utility, as polling is limited.

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

This is cool, thank you for your contribution! It closes this ticket and uses the solution I suggested over there: #1316.

I just have two change requests:

  • Could you rename *WithoutPolling -> *Sync to follow the naming given by the backend? Also polling is an implementation detail that can change over time.
  • Can you deduplicate the code for all cases such that e.g. broadcastTx uses broadcastTxSync for the first steps of the operation (accepted by the node)?

changed function names to follow the naming given by the backend.
@DavideSegullo
Copy link
Contributor Author

This is cool, thank you for your contribution! It closes this ticket and uses the solution I suggested over there: #1316.

I just have two change requests:

  • Could you rename *WithoutPolling -> *Sync to follow the naming given by the backend? Also polling is an implementation detail that can change over time.
  • Can you deduplicate the code for all cases such that e.g. broadcastTx uses broadcastTxSync for the first steps of the operation (accepted by the node)?

Yes, it seems fine to me, I did some changes, is it okay now?

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Diff LGTM, thanks!

Could you add one test for each new function (one describe block per function)? No need to copy all the cases we have for signAndBroadcast but at least one happy path to ensure this does not break over time.

And then we need a CHANGELOG entry in the Unreleased section for this work.

Once we have the two things, this can get shipped in 0.31.0. Thank you!

@webmaster128 webmaster128 added this to the 0.31.0 milestone May 25, 2023
@DavideSegullo
Copy link
Contributor Author

DavideSegullo commented May 25, 2023

Ofc, I have added some tests, please tell me if they are enought!

added block time sleep to avoid sequence mismatch.
changed test msg from delegate to send.
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Let's remove those delegation tests. Otherwise the staking about in some other test fails. This is not great right now as the staking balance tests depends on the MsgDelegate tests. But let's move on here and get this in.

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>
@DavideSegullo
Copy link
Contributor Author

Let's remove those delegation tests. Otherwise the staking about in some other test fails. This is not great right now as the staking balance tests depends on the MsgDelegate tests. But let's move on here and get this in.

Done, I approved it!

@webmaster128
Copy link
Member

Okay it seems like only a linting issue left. Could you fix them (with yarn && yarn lint-fix in the repo root)?

@webmaster128
Copy link
Member

Seems like I can't push here. Could you allow changes by maintaners? Then I can write a CHANGELOG entry, push here and merge to get it in the 0.31 release.

@DavideSegullo
Copy link
Contributor Author

Seems like I can't push here. Could you allow changes by maintaners? Then I can write a CHANGELOG entry, push here and merge to get it in the 0.31 release.

Do you know how I can enable it?

Okay it seems like only a linting issue left. Could you fix them (with yarn && yarn lint-fix in the repo root)?

Done

@webmaster128
Copy link
Member

Thanks, see here: bisq-network/style#4

@DavideSegullo
Copy link
Contributor Author

Thanks, see here: bisq-network/style#4

I tried it, but I don't see "Allow edits from maintainers" button

@DavideSegullo
Copy link
Contributor Author

DavideSegullo commented Jun 15, 2023

Well, I found out that the problem is related to cross-org PRs: https://github.com/orgs/community/discussions/5634

I'll try to give you permissions on that repo.

Update: I gave you the maintainer permissions in the repo

@webmaster128 webmaster128 merged commit e37b614 into cosmos:main Jun 19, 2023
@webmaster128 webmaster128 deleted the nabla/improve-sign_dx branch June 19, 2023 13:02
@webmaster128
Copy link
Member

Thanks a lot. Will be shipped as part of 0.31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants