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

review guard condition on self withdrawl redemption #2001

Merged
merged 3 commits into from
Aug 4, 2020

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Aug 4, 2020

Issue Number

#1965

Overview

  • 728ab75
    📍 change integration scenario to always allow redeeming from self
    This makes it a lot easier for clients to deal with rewards. One can always give 'self' and simply let the backend decides whether or not rewards should be redeemed. For external wallets however, it could be quite counter-intuitive to succeed in the case there are no rewards in the target wallet so we still error in that case.

  • 278c9a3
    📍 move rewards redemption guard to only apply to external redemptions
    As per previous commit.

  • 029da7f
    📍 improve and adjust API documentation on withdrawal redemption

Comments

KtorZ added 3 commits August 4, 2020 15:48
  This makes it a lot easier for clients to deal with rewards. One can always give 'self' and simply let the backend decides whether or not rewards should be redeemed. For external wallets however, it could be quite counter-intuitive to succeed in the case there are no rewards in the target wallet so we still error in that case.
@KtorZ KtorZ added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Aug 4, 2020
@KtorZ KtorZ requested a review from paweljakubas August 4, 2020 13:57
@KtorZ KtorZ self-assigned this Aug 4, 2020
@KtorZ
Copy link
Member Author

KtorZ commented Aug 4, 2020

bors try

iohk-bors bot added a commit that referenced this pull request Aug 4, 2020
Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

LGTM. Before this PR, we had the check always ON, for both ExternalWithdrawal and SelfWithdrawal cases when withdrawal = 0 and this could impede SelfWithdrawal txs we don't want to be stopped. Now we will only do it for ExternalWithdrawal case, meaning we will not end up with ErrWithdrawalNotWorth error in SelfWithdrawal case. Which is reasonable change.

@KtorZ
Copy link
Member Author

KtorZ commented Aug 4, 2020

Hydra is failing on the MacOS integration test, failure seems completely unrelated. So, moving on 👍

@KtorZ KtorZ merged commit e1890db into master Aug 4, 2020
@KtorZ KtorZ deleted the KtorZ/1965/redeem-itn-rewards branch August 4, 2020 15:46
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 4, 2020

try

Timed out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants