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.

Sign up for GitHub

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

Jump to bottom

Handle too small outputs/changes #1916

Merged
merged 10 commits into from
Jul 18, 2020
Merged

Handle too small outputs/changes #1916

merged 10 commits into from
Jul 18, 2020

Conversation

paweljakubas
Copy link
Contributor

Issue Number

#1914

Overview

  • I have added the error handling situation when any TxOut is smaller than minimumUTxOvalue
  • I have added guardSelect and used in selectCoinsForPayment
  • I have also make sure it is in estimateFeeForPayment

Comments

@paweljakubas paweljakubas self-assigned this Jul 16, 2020
@paweljakubas paweljakubas requested review from rvl and piotr-iohk July 16, 2020 11:59
rvl
rvl approved these changes Jul 16, 2020
View reviewed changes
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

LGTM

lib/core/src/Cardano/Wallet.hs Outdated
Comment on lines 2170 to 2179
guardSelect
:: Coin
-> NonEmpty TxOut
-> Either ErrTxOutTooSmall ()
guardSelect minUtxoValue txouts = do
let invalidTxOuts =
filter (\(TxOut _ out) -> out < minUtxoValue) $ NE.toList txouts
let getVals = map (\(TxOut _ (Coin c)) -> c)
unless (L.null invalidTxOuts) $
Left (ErrTxOutTooSmall (getCoin minUtxoValue) (getVals invalidTxOuts) )
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this might be easier to call? -- the ExceptT is built in.

Suggested change
guardSelect
:: Coin
-> NonEmpty TxOut
-> Either ErrTxOutTooSmall ()
guardSelect minUtxoValue txouts = do
let invalidTxOuts =
filter (\(TxOut _ out) -> out < minUtxoValue) $ NE.toList txouts
let getVals = map (\(TxOut _ (Coin c)) -> c)
unless (L.null invalidTxOuts) $
Left (ErrTxOutTooSmall (getCoin minUtxoValue) (getVals invalidTxOuts) )
guardSelect
:: Coin
-> NonEmpty TxOut
-> ExceptT m ErrSelectForPayment ()
guardSelect minUtxoValue txouts
| outputsAreAllBigEnough = pure ()
| otherwise = throwE $ ErrSelectForPayment $ ErrTxOutTooSmall (getCoin minUtxoValue) (getVals invalidTxOuts)
where
outputsAreAllBigEnough = not (L.null invalidTxOuts)
invalidTxOuts = filter (\(TxOut _ out) -> out < minUtxoValue) $ NE.toList txouts
getVals = map (\(TxOut _ (Coin c)) -> c)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably. here I just wanted to be aligned with other guards and more fit to the style already existent. But I will add property to guardSelect in coming commit

@piotr-iohk
Copy link
Contributor

Testing on shelley_testnet on this branch, found 1 issue:

wallet with balance 11 ADA with two utxos (10 ADA and 1 ADA). trying to make tx from such wallet for 10ADA results with an error:

Code = 500,
Message = {"code":"created_invalid_transaction","message":"That's embarrassing. It looks like I've created an invalid transaction that could not be parsed by the node. Here's an error message that may help with debugging: ApplyTxError [LedgerFailure (UtxowFailure (UtxoFailure (OutputTooSmallUTxO [TxOut (Addr Testnet (KeyHashObj (KeyHash 989a5e4a820f030b41cd7f7a108d2aa10c7062703e79679797071681)) (StakeRefBase (KeyHashObj (KeyHash 3f2ea40ab5c9a07c1c3800ef0c51ae474b70e163d975daab2d40db39)))) (Coin 825655)])))]"}

Steps to reproduce:
minimumUtxoValue = 1 ADA

  1. make to transactions to walletA:
  • first transaction for 1 ADA
  • second transaction for 10 ADA
  1. From walletA try to make transaction for 10 ADA

this 👆 error 500 occurs.

@paweljakubas
Copy link
Contributor Author

@piotr-iohk I hope this change will help 87dc401 now we are checking after fee adjustement, looking at coin selection

@paweljakubas paweljakubas changed the title Handle too small outputs Handle too small outputs/changes Jul 16, 2020
@paweljakubas paweljakubas changed the title Handle too small outputs/changes Handle too small outputs/changes Jul 16, 2020
@piotr-iohk
Copy link
Contributor

Tested on shelley, icarus and byron wallets on shelley testnet with minimumUtxoValue = 1 ada.
In all 403 cases below the response is

Code = 403,
Message = {"code":"u_tx_o_too_small",
"message":"I'm unable to construct the given transaction as some outputs or changes are too small! 
Each output and change is expected to be >= 1000000 Lovelace.
In the current transaction the following pieces are not satisfying this condition : [invalid Outputs List] ."}

wallet balance = big arbitrary balance

  • sending 1 ada => OK ✔️

  • sending 0,9 => 403 ✔️

  • sending multi output [1; 0,9; 0,9] => 403, and invalid inputs mentioned ✔️

  • sending multi output [1; 1; 1] => OK ✔️

  • fee estimation for 1 ada => OK ✔️

  • fee estimation for 0,9 => 403 ✔️

  • fee estimation for multi output [1; 0,9; 0,9] => 403, and invalid inputs mentioned ✔️

  • fee estimation for multi output [1; 1; 1] => OK ✔️

wallet balance 11 ada (utxo1 = 10ada, utxo2 = 1ada)

  • sending 10 => 403 ✔️
  • fee estimation 10 => 403 ✔️

The second scenario may be quite misleading for wallet owner, as he has balance = 11ada and cannot send 10ada, because the change of the transaction is below 1 ada. The information about the change is in the error message. Not sure if we can do more...

@piotr-iohk
Copy link
Contributor

I'd suggest also adding unit test for that.

@paweljakubas
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Jul 16, 2020
@iohk-bors @paweljakubas
Merge #1916
71bb551 
1916: Handle too small outputs/changes   r=paweljakubas a=paweljakubas

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->
#1914 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have added the error handling situation when any `TxOut` is smaller than `minimumUTxOvalue`
- [x] I have added `guardSelect` and used in `selectCoinsForPayment`
- [x] I have also make sure it is in `estimateFeeForPayment`

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Pawel Jakubas <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 16, 2020

Build failed

@paweljakubas paweljakubas force-pushed the paweljakubas/1914/handle-too-small-outputs branch from ba8b836 to 29d8a9d Compare July 17, 2020 10:52
@paweljakubas
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request Jul 17, 2020
@iohk-bors
Try #1916:
07a5a9d
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 17, 2020

try

Build failed

@paweljakubas paweljakubas force-pushed the paweljakubas/1914/handle-too-small-outputs branch from 3e921cf to 6ea2d92 Compare July 17, 2020 11:43
@paweljakubas
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request Jul 17, 2020
@iohk-bors
Try #1916:
c950b7f
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 17, 2020

try

Build failed

@paweljakubas
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request Jul 17, 2020
@iohk-bors
Try #1916:
2783752
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 17, 2020

try

Build failed

@paweljakubas paweljakubas force-pushed the paweljakubas/1914/handle-too-small-outputs branch from 6ea2d92 to 73eb89a Compare July 17, 2020 14:43
@paweljakubas
Copy link
Contributor Author

bors try

iohk-bors bot added a commit that referenced this pull request Jul 17, 2020
@iohk-bors
Try #1916:
e441ba3
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 17, 2020

try

Build succeeded

@paweljakubas paweljakubas force-pushed the paweljakubas/1914/handle-too-small-outputs branch from 73eb89a to 5d1666a Compare July 18, 2020 07:11
@paweljakubas
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 18, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit 652e32e into master Jul 18, 2020
@iohk-bors iohk-bors bot deleted the paweljakubas/1914/handle-too-small-outputs branch July 18, 2020 08:02
@piotr-iohk piotr-iohk mentioned this pull request Jul 18, 2020
Closed
@paweljakubas paweljakubas mentioned this pull request Jul 18, 2020
Closed
KtorZ
KtorZ reviewed Jul 21, 2020
View reviewed changes
lib/core/src/Cardano/Wallet/Api/Server.hs
ErrUTxOTooSmall minUtxoValue invalidUTxO ->
apiError err403 UtxoTooSmall $ mconcat
[ "I'm unable to construct the given transaction as some "
, "outputs or changes are too small! Each output and change is "
Copy link
Member

Choose a reason for hiding this comment

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

small note: we shouldn't be talking about change here because there's nothing users can do about change. We are actually generating them and, having changes that are too small here would mean that we messed up. Yet, for outputs, this error looks totally legit indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KtorZ KtorZ KtorZ left review comments

@rvl rvl rvl approved these changes

@piotr-iohk piotr-iohk Awaiting requested review from piotr-iohk

Assignees

@paweljakubas paweljakubas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@paweljakubas @piotr-iohk @rvl @KtorZ