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

More integration tests for construct tx endpoint #2751

Merged
merged 5 commits into from
Jul 7, 2021

Conversation

piotr-iohk
Copy link
Contributor

@piotr-iohk piotr-iohk commented Jul 5, 2021

Issue Number

ADP-909

Overview

Integration tests covering:

  • More malformed examples
  • Only validate_interval
  • Tx with only metadata
  • Tx with only withdrawal from self and external
  • Only single output tx ⚠️ pending - outputs are empty
  • Only multi-output tx ⚠️ pending - outputs are empty
  • Multi asset transactions ⚠️ pending - outputs are empty
  • Join and quit pool
    • join ⚠️ pending - deposits are empty
    • quit ⚠️ pending - can quit when you didn't join
    • errMsg404NoSuchPool ⚠️ pending - error not returned when should be
  • Minting
  • validate_interval on different transactions (payments, delegations, mint)
    • invalid_before / invalid_hereafter ⚠️ pending - can be < 0 but shouldn't
  • Transaction with all (payments, delegations, mint, metadata, withdrawal, validate_interval)

Comments / Issues

  • Should construct ep return 200 instead of 202 (coin selection returns 200)
  • amount field would be probably handy to have in the response
  • Output field on response is empty ("output": []) on payment transactions, which seems like a bug (to be addressed in ADP-985)... for now tests marked as pendingWith
  • Deposit is empty on construct tx response on joining pool and shouldn't
  • You can quit pool before even joining
  • errMsg404NoSuchPool is not returned when trying to join absent pool

@piotr-iohk piotr-iohk self-assigned this Jul 5, 2021
@piotr-iohk piotr-iohk added the TESTS Adding additional tests label Jul 5, 2021
@piotr-iohk piotr-iohk force-pushed the piotr/testing-construct-tx-ep branch from e40bef5 to c66f572 Compare July 6, 2021 10:56
@piotr-iohk piotr-iohk requested a review from paweljakubas July 6, 2021 15:59
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

@piotr-iohk
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Jul 7, 2021
2751: More integration tests for construct tx endpoint r=piotr-iohk a=piotr-iohk

# Issue Number

ADP-909


# Overview

Integration tests covering:

- [x] More malformed examples
- [x] Only validate_interval
- [x] Tx with only metadata
- [x] Tx with only withdrawal from self and external
- [x] Only single output tx ⚠️ `pending` - outputs are empty 
- [x] Only multi-output tx ⚠️ `pending` - outputs are empty 
- [x] Multi asset transactions ⚠️ `pending` - outputs are empty 
- [x] Join and quit pool
  - join ⚠️ `pending` - deposits are empty
  - quit ⚠️ `pending` - can quit when you didn't join
  - errMsg404NoSuchPool ⚠️ `pending` - error not returned when should be
- [ ] ~Minting~
- [x] validate_interval on different transactions (payments, delegations, mint)
  - invalid_before / invalid_hereafter ⚠️ `pending` -  can be < 0 but shouldn't
- [x] Transaction with all (payments, delegations, mint, metadata, withdrawal, validate_interval)


# Comments / Issues

 - Should construct ep return `200` instead of `202` (coin selection returns `200`)
 - `amount` field would be probably handy to have in the response  
 - Output field on response is empty (`"output": []`) on payment transactions, which seems like a bug (to be addressed in ADP-985)... for now tests marked as `pendingWith`
 - Deposit is empty on construct tx response on joining pool and shouldn't
 - You can quit pool before even joining
 - errMsg404NoSuchPool is not returned when trying to join absent pool


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

iohk-bors bot commented Jul 7, 2021

Build failed:



cardano-wallet                 > Failures:
--
  | cardano-wallet                 >
  | cardano-wallet                 >   src/Test/Integration/Scenario/CLI/Shelley/Wallets.hs:285:56:
  | cardano-wallet                 >   1) CLI Specifications, SHELLEY_CLI_WALLETS, WALLETS_CREATE_04 - Wallet names invalid, name is too short: expected at least 1 character
  | cardano-wallet                 >        uncaught exception: IOException of type ResourceVanished
  | cardano-wallet                 >        fd:146: hFlush: resource vanished (Broken pipe)
  | cardano-wallet                 >
  | cardano-wallet                 >   To rerun use: --match "/CLI Specifications/SHELLEY_CLI_WALLETS/WALLETS_CREATE_04 - Wallet names invalid/name is too short: expected at least 1 character/"
  | cardano-wallet                 >
  | cardano-wallet                 > Randomized with seed 1377329000
  | cardano-wallet                 >
  | cardano-wallet                 > Finished in 907.3185 seconds, used 996.7970 seconds of CPU time
  | cardano-wallet                 > 836 examples, 1 failure, 21 pending


#2753

Piotr Stachyra added 4 commits July 7, 2021 14:34
@piotr-iohk piotr-iohk force-pushed the piotr/testing-construct-tx-ep branch from ebda326 to f95f65e Compare July 7, 2021 12:38
@piotr-iohk
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Jul 7, 2021
2751: More integration tests for construct tx endpoint r=piotr-iohk a=piotr-iohk

# Issue Number

ADP-909


# Overview

Integration tests covering:

- [x] More malformed examples
- [x] Only validate_interval
- [x] Tx with only metadata
- [x] Tx with only withdrawal from self and external
- [x] Only single output tx ⚠️ `pending` - outputs are empty 
- [x] Only multi-output tx ⚠️ `pending` - outputs are empty 
- [x] Multi asset transactions ⚠️ `pending` - outputs are empty 
- [x] Join and quit pool
  - join ⚠️ `pending` - deposits are empty
  - quit ⚠️ `pending` - can quit when you didn't join
  - errMsg404NoSuchPool ⚠️ `pending` - error not returned when should be
- [ ] ~Minting~
- [x] validate_interval on different transactions (payments, delegations, mint)
  - invalid_before / invalid_hereafter ⚠️ `pending` -  can be < 0 but shouldn't
- [x] Transaction with all (payments, delegations, mint, metadata, withdrawal, validate_interval)


# Comments / Issues

 - Should construct ep return `200` instead of `202` (coin selection returns `200`)
 - `amount` field would be probably handy to have in the response  
 - Output field on response is empty (`"output": []`) on payment transactions, which seems like a bug (to be addressed in ADP-985)... for now tests marked as `pendingWith`
 - Deposit is empty on construct tx response on joining pool and shouldn't
 - You can quit pool before even joining
 - errMsg404NoSuchPool is not returned when trying to join absent pool


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

iohk-bors bot commented Jul 7, 2021

Build failed:

#expected

@piotr-iohk piotr-iohk force-pushed the piotr/testing-construct-tx-ep branch from f95f65e to ad9cd19 Compare July 7, 2021 13:27
@piotr-iohk
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Jul 7, 2021
2751: More integration tests for construct tx endpoint r=piotr-iohk a=piotr-iohk

# Issue Number

ADP-909


# Overview

Integration tests covering:

- [x] More malformed examples
- [x] Only validate_interval
- [x] Tx with only metadata
- [x] Tx with only withdrawal from self and external
- [x] Only single output tx ⚠️ `pending` - outputs are empty 
- [x] Only multi-output tx ⚠️ `pending` - outputs are empty 
- [x] Multi asset transactions ⚠️ `pending` - outputs are empty 
- [x] Join and quit pool
  - join ⚠️ `pending` - deposits are empty
  - quit ⚠️ `pending` - can quit when you didn't join
  - errMsg404NoSuchPool ⚠️ `pending` - error not returned when should be
- [ ] ~Minting~
- [x] validate_interval on different transactions (payments, delegations, mint)
  - invalid_before / invalid_hereafter ⚠️ `pending` -  can be < 0 but shouldn't
- [x] Transaction with all (payments, delegations, mint, metadata, withdrawal, validate_interval)


# Comments / Issues

 - Should construct ep return `200` instead of `202` (coin selection returns `200`)
 - `amount` field would be probably handy to have in the response  
 - Output field on response is empty (`"output": []`) on payment transactions, which seems like a bug (to be addressed in ADP-985)... for now tests marked as `pendingWith`
 - Deposit is empty on construct tx response on joining pool and shouldn't
 - You can quit pool before even joining
 - errMsg404NoSuchPool is not returned when trying to join absent pool


Co-authored-by: Piotr Stachyra <[email protected]>
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 - thanks.

it "TRANS_NEW_CREATE_01b - Validity interval only is not allowed" $ \ctx -> runResourceT $ do
wa <- fixtureWallet ctx
let validityInterval = Json [json|{
"validity_interval": {
Copy link
Contributor

Choose a reason for hiding this comment

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

@paweljakubas We should also add a simple validation (invalid before <= invalid hereafter) of the validity interval to the API JSON parser.

let payload = Json [json|{
"withdrawal": "self",
"validity_interval": {
"invalid_before": "unspecified",
Copy link
Contributor

Choose a reason for hiding this comment

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

It should be that omission of the interval bound, or just setting it to null means "unspecified" -- no need to use a special string.

@@ -1363,7 +1363,7 @@ x-transactionTTL: &transactionTTL
x-unspecified: &unspecified
type: string
enum:
- uspecified
- unspecified
Copy link
Contributor

Choose a reason for hiding this comment

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

@paweljakubas we should remove this "unspecified" and use null/undefined/missing to denote unspecified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we've discussed this with Paweł and for now just added some tests (also covering other "failing" scenarios) and make them "pending"), such that then can be enabled later once the issues are fixed. Findings also listed here -> https://jira.iohk.io/browse/ADP-909.

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 7, 2021

Build failed:

#expected (hlint)

@piotr-iohk piotr-iohk force-pushed the piotr/testing-construct-tx-ep branch from ad9cd19 to 3567a08 Compare July 7, 2021 14:14
@piotr-iohk
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jul 7, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 4222028 into master Jul 7, 2021
@iohk-bors iohk-bors bot deleted the piotr/testing-construct-tx-ep branch July 7, 2021 16:00
WilliamKingNoel-Bot pushed a commit that referenced this pull request Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TESTS Adding additional tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants