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

colw/2637 withdraw from 1-5 validators at a time #2647

Merged
merged 21 commits into from
May 23, 2019

Conversation

colw
Copy link
Contributor

@colw colw commented May 21, 2019

Closes #2637

Description:

Withdraw in one of two ways:

  • Global withdraw: Will withdraw from the 5 validators with the highest rewards.
  • Validator page withdraw: Withdraw from single validator only

Screenshot 2019-05-23 at 11 50 15

If there are no rewards from a given validator, the button is disabled:
Screenshot 2019-05-23 at 11 51 46

The modal supports both variations and displays a message when withdrawing globally.
Screenshot 2019-05-23 at 11 49 17

Message: Note: Lunie withdraws only the top 5 rewards at a time.

Thank you! 🚀


For contributor:

  • Added changes entries. Run yarn changelog for a guided process.
  • Reviewed Files changed in the github PR explorer
  • Attach screenshots of the UI components on the PR description (if applicable)
  • Scope of work approved for big PRs

For reviewer:

  • Manually tested the changes on the UI

colw added 3 commits May 22, 2019 00:08
- Add address to `withdrawAllRewards` action
    If present, add array of single validator address
    else use all validators with rewards
- Add button below delegation / undelegation validator section
- Create new Modal for single withdrawal. Streamlined from multiple
  variant
@colw colw changed the title Colw/2637 withdraw from single validator colw/2637 withdraw from single validator May 22, 2019
type: String,
required: true
},
rewards: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about the ActionModal webcomponent, does it make sense to make the rewards an input? Or would we wrap this in a controller?

Copy link
Contributor Author

@colw colw May 22, 2019

Choose a reason for hiding this comment

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

Yep, I thought about this. I believe wrapping any future ActionModal web component in a controller is better.

IMHO, the Modals should be 'dumb' views, and at most receive data and forward user actions, but that's it. Separation of concerns.

Note: We should incrementally move all views to this approach.

colw added 11 commits May 22, 2019 12:48
- Remove some cruft
- Keep data conversions out of the modal
- Clean withdraw all modal
- Replace WithdrawAll modal with singular modal
Remove mapActions in favour so of calling $store
directly.
@colw colw changed the title colw/2637 withdraw from single validator colw/2637 withdraw from 1-5 validators at a time May 23, 2019
@colw colw marked this pull request as ready for review May 23, 2019 09:53
@colw colw requested a review from jbibla as a code owner May 23, 2019 09:53
<span class="input-suffix">{{ denom | viewDenom }}</span>
<TmField id="amount" :value="rewards | atoms | fullDecimals" readonly />
</TmFormGroup>
<span v-if="!validatorAddress" class="form-message">
Copy link
Collaborator

Choose a reason for hiding this comment

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

should the message show always or only when the user actually has more then 5 delegations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♂ You're right!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, is it necessary? We could leave it there to inform the user regardless?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally like to reduce noise that is not important to users. @jbibla preferences?

Copy link
Contributor Author

@colw colw May 23, 2019

Choose a reason for hiding this comment

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

  • It could have a minor impact on the message wording. The current phrasing is generic.

src/utils/index.js Outdated Show resolved Hide resolved
) {
const top5Delegations = getTop5Delegations(getters.committedDelegations)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should only limit to 5 delegations if the user is using the Ledger Nano S, but we can figure this out once we have more signers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. There's some time to spare before the Nano X is in wide distribution, and we'll need to detect the difference. Is that right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep

@colw colw force-pushed the colw/2637-withdraw-from-single-validator branch from d1180af to 0ddf8a1 Compare May 23, 2019 13:42
@faboweb faboweb merged commit 6ac671f into develop May 23, 2019
@faboweb faboweb deleted the colw/2637-withdraw-from-single-validator branch May 23, 2019 16:09
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.

Ledger Nano S is not able to process big transactions (i.e. many withdrawels)
3 participants