Skip to content
This repository was archived by the owner on Mar 1, 2019. It is now read-only.

Improve instructions (and testing) about multi-output transactions #190

Closed
2 tasks done
KtorZ opened this issue Jan 2, 2019 · 3 comments
Closed
2 tasks done

Improve instructions (and testing) about multi-output transactions #190

KtorZ opened this issue Jan 2, 2019 · 3 comments
Assignees
Labels
ACCEPTED ADR has been accepted
Milestone

Comments

@KtorZ
Copy link
Contributor

KtorZ commented Jan 2, 2019

Context

The new coin selection algorithm has some limitations that aren't clearly stated from the documentation (nor in the specification actually). And a few points about multi-outputs transactions are still unresolved.
The coin selection can't split a UTxO across multiple outputs. In order to make a multi-outputs transaction, there should be several available UTxO.

Decision

When trying to make a transaction with more outputs than available UTxO, the API fails with CoinSelHardErrUtxoExhausted which isn't really helpful from a user point of view. Ideally, we would improve the error message and suggest a possible fix or, way of approaching the error (by suggesting to have a look at the utxo fragmentation through the utxo statistics endpoint).

Also, we want to add integration tests illustrating the scenario above (failing and successful one) and, with this, extend the API documentation to explain how to deal with multi-outputs transactions and what are the limitations.

Acceptance Criterias

  1. The API must contain instructions about fragmenting a wallet UTxO
  2. The API must contain instructions about creating multi-outputs transactions, suggesting an ideal and maximum level of fragmentation for exchanges.
  3. Some Integration tests must illustrate the following scenarios:
    • A multi-output transaction fails because there's not enough UTxO to cover for all outputs
    • A multi-output transaction succeeds
  4. The API should return a more meaningful error when the case for (failing) multi-output is encountered, suggesting the user to have a look at /api/v1/wallets/{walletId}/statistics

Development Plan

  • Realize what is required above
  • add boundary tests to multioutput transaction suite

PR

Number Base
#272 develop
#288 develop
#289 develop
#290 develop
input-output-hk/cardano-sl#4058 develop
#298 develop

QA

When the number of transaction outputs is greater than the number of utxo new error is triggered two tests in Transaction section of integration tests are added that cover the situation.

When the number of transaction outputs is greater than the number of utxo new error is triggered . Previously it the following error was triggered:

expectWalletError (NotEnoughMoney (ErrAvailableBalanceIsInsufficient 0)

which is a bit misleading.
Now, the following error is expected:

UtxoNotEnoughFragmented (Client.ErrUtxoNotEnoughFragmented 1 Client.msgUtxoNotEnoughFragmented)

which here indicates situation that there is 1 utxo lacking to cover all outputs of a given transaction.
Here, we are expecting the following msg :

{    
    "status": "error", 
    "diagnostic": {        
        "details": {            
            "help": "Utxo is not enough fragmented to handle the number of outputs of this transaction. Query /api/v1/wallets/{walletId}/statistics/utxos endpoint for more information",            
            "missingUtxos": 1        
        }    
    },    
    "message": "UtxoNotEnoughFragmented"
}

The corresponding tests were added to integration test suite (cardano-wallet/test/integration/Test/Integration/Scenario/Transactions.hs) :

scenario "proper fragmentation of utxo allows for multi-output transaction"
scenario "not enough fragmentation of utxo forbids multi-output transaction"

Situations were this error is detected :

  1. when the fragmentation of utxo is smaller (ie., the number of utxo) than the number of outputs in a transaction. How to reproduce scenario:
  • create wallet S with nonzero initial coin
  • create other 2 wallets (D1 and D2) with zero initial coin
  • send money (make sure to have enough money to cover fees, eg. 100000 in wallet S) eg 10 and 11, to two default accounts of D1 and D2 (multioutput means here two-output).
    (see added integration tests for details)
  1. There is small breaking change here when it comes to scenario :
scenario "payment fails when wallet has no funds"

Here, one should expect not NotEnoughMoney but UtxoNotEnoughFragmented
Why, because we try to send transaction from source wallet that does not have utxo to one output and here we trigger UtxoNotEnoughFragmented
How to reproduce scenario :

  • create wallet with no funds and try to send any transaction
@KtorZ KtorZ added the PROPOSED ADR just proposed label Jan 3, 2019
@KtorZ KtorZ changed the title TODO: Multi-output transactions Improve instructions (and testing) about multi-output transactions Jan 3, 2019
@KtorZ KtorZ added ACCEPTED ADR has been accepted and removed PROPOSED ADR just proposed labels Jan 3, 2019
@KtorZ KtorZ added this to the Cardano 1.5 milestone Jan 25, 2019
@paweljakubas paweljakubas self-assigned this Jan 28, 2019
@KtorZ
Copy link
Contributor Author

KtorZ commented Jan 31, 2019

@paweljakubas I am just thinking about it but I believe we forgot to update the documentation about this. Unfortunately, since errors aren't included in Servant types, we don't really generate automatically any docs for them and they still have to be added manually in here:

https://github.com/input-output-hk/cardano-wallet/blob/develop/src/Cardano/Wallet/API/V1/Swagger.hs#L304-L340

@piotr-iohk
Copy link
Contributor

piotr-iohk commented Jan 31, 2019

@paweljakubas - lgtm 👍 except for documentation update mentioned above ^^. As discussed, I have also added one additional test triggering UtxoNotEnoughFragmented -> #288 (CC: @KtorZ )

@paweljakubas paweljakubas mentioned this issue Jan 31, 2019
3 tasks
iohk-bors bot referenced this issue in input-output-hk/cardano-sl Feb 1, 2019
4058: Support UtxoNotEnoughFragmented error r=KtorZ a=paweljakubas

Add multioutput integration tests

Fix Lens accesses and confusion between WalAddress vs WalletAddress vs V1 Address

## Description

This is backport of cardano-foundation/cardano-wallet#190

## Linked issue

<!--- Put here the relevant issue from YouTrack -->



Co-authored-by: Pawel Jakubas <[email protected]>
@KtorZ KtorZ added the RESTROSPECTIVE ADR needs restrospective session label Feb 4, 2019
@KtorZ
Copy link
Contributor Author

KtorZ commented Feb 4, 2019

Closing this one as the documentation point has been covered and, more test addded. Also successfully backported into cardano-sl/develop

@KtorZ KtorZ closed this as completed Feb 4, 2019
@KtorZ KtorZ removed the RESTROSPECTIVE ADR needs restrospective session label Feb 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ACCEPTED ADR has been accepted
Projects
None yet
Development

No branches or pull requests

3 participants