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

allow splitting withdrawal txs #4384

Merged
merged 11 commits into from
May 23, 2019
Merged

Conversation

jleni
Copy link
Member

@jleni jleni commented May 21, 2019

Due to HSM limitations (Ledger Nano S), big transactions such as withdrawing rewards from a large number of validators, may be rejected to due memory constraints.

This PR introduces an additional flag that allows splitting the withdrawal messages and generates several transactions. Users can then define the maximum number of messages that they want to use.

This is related to: https://github.com/cosmos/ledger-cosmos/issues/143
While we have extended limits to Nano X in the next release, it is important to provide an alternative to users of Nano S.

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: clog add [section] [stanza] [message]
  • rereviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@jleni jleni marked this pull request as ready for review May 21, 2019 09:00
x/distribution/client/cli/tx.go Outdated Show resolved Hide resolved
x/distribution/client/cli/tx.go Outdated Show resolved Hide resolved
x/distribution/client/cli/tx.go Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

One minor nit, otherwise LGTM 👌

I may actually renege my statement on making this functionality only available to this command. Would you still be in favor of applying this to all commands?

x/distribution/client/cli/tx.go Outdated Show resolved Hide resolved
Co-Authored-By: Alexander Bezobchuk <[email protected]>
@jleni jleni requested a review from alexanderbez May 22, 2019 19:20
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

good changes! should tidy the code just a wee bit

x/distribution/client/cli/tx.go Outdated Show resolved Hide resolved
@jleni jleni requested a review from rigelrozanski May 23, 2019 21:42
@rigelrozanski rigelrozanski merged commit 68036ec into cosmos:master May 23, 2019
@jleni jleni deleted the jleni/tx_splitting branch May 24, 2019 13:09
@alexanderbez alexanderbez mentioned this pull request May 24, 2019
5 tasks
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.

5 participants