Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

[CBR-398] Adding integration test for redeemADA functionality #3525

Merged

Conversation

paweljakubas
Copy link
Contributor

@paweljakubas paweljakubas commented Aug 31, 2018

Description

integration test for redeemADA. The tests do the following:

  1. Wallet is created.
  2. New account is added to the wallet.
  3. The check that the account balance is 0 is enforced.
  4. Redemption value is constructed (using fake avvv seed available for dev setup- which is used as secret key in Ed25519 which is used for redemption stuff)
  5. redeemAda is called
  6. Corresponding transaction index is checked.
  7. Balance of the account is checked if it is increased by 100000.
  8. Finally, redeemAda is tried again with the same redemption value and it is expected to end up with error Request error (Cannot send redemption transaction: Redemption address balance is 0)"

Remark: checking redeemADA with the same redemption value - I expect error here and at this moment the below works:
ClientWalletError (UnknownError "Request error (Cannot send redemption transaction: Redemption address balance is 0)")

When new data layer is switched on: catching RedeemAdaNotAvailable Address should be enforced

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CBR-398

Type of change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • 🛠 New feature (non-breaking change which adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • 🔨 New or improved tests for existing code
  • ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

nix-build release.nix -A tests.walletIntegration

Screenshots (if available)

@paweljakubas
Copy link
Contributor Author

paweljakubas commented Aug 31, 2018

I checked logs in integrationTests and the error comes from Transactions test suites (meaning test added in this PR come through):

    Response #122 NotEnoughMoney 155423 0.00058147s
NotEnoughMoney 155423
[wallet.api:INFO:ThreadId 1921] [2018-08-31 09:36:40.85 UTC] 
GET Request #123
    :> api
    :> v1
    :> wallets
    :> 'walletId' field: Ae2tdPwUPEZBgastrNueQdnWqRuBEgUCT793MtJ7PRWKNTYBdsEr7HJBb9d
    :> accounts
    :> pagination: page=1, per_page=50
[wallet.api:INFO:ThreadId 1921] [2018-08-31 09:36:40.85 UTC] 
    Response #123 OK 0.005109205s > 
	status=success
	meta={ pagination=1/1 total=4 per_page=50 }
	data=[{ index=2147483648 name=Initial account addresses=[{ id=DdzFFzCqrhstCJefWv8UE91bnfzUPYYj4xyH6uxyz8TTFkjQJw6sLMTdELpY7wrxt7SFPSqWRzUQJKmr8W6Bwqr2p5rQ5tVdZwJ2T9ek used=True changeAddress=False }] amount=3749999948650 coin(s) walletId=Ae2tdPwUPEZBgastrNueQdnWqRuBEgUCT793MtJ7PRWKNTYBdsEr7HJBb9d }, { index=2589649600 name=hello addresses=[{ id=DdzFFzCqrhss9c7yYJ3HeKMK7uHJyN5xnp4vfboD3yJPEf5DnKe5kuNiWdZmvFpKYGwsVJKMhUokYy7FE3tMQ9iFv9pUEXLcpjydFp8T used=False changeAddress=False }, { id=DdzFFzCqrht3wV83e9i5ee7VeWWKjMFwhvCYq6EyLR7b2ACLricXG8RM1J8pUZpeKh5vudpxBcwgP7BjwNqnZ1h3WEu1JHefnY2DvZMC used=False changeAddress=False }] amount=0 coin(s) walletId=Ae2tdPwUPEZBgastrNueQdnWqRuBEgUCT793MtJ7PRWKNTYBdsEr7HJBb9d }, { index=2746514027 name=Y񧓓3񳘵񞹏~󌽖7I􈡗�`A_N addresses=[{ id=DdzFFzCqrht5C83f7P6sbdSFNW2kc5H3wR2H96KhPezhVTtzYxtFmC3fwrKn1AzWdrRWb5Z6fWoGYFpUWjRbh7Yz8GTpo5Z1UDwWxwJp used=True changeAddress=False }] amount=5 coin(s) walletId=Ae2tdPwUPEZBgastrNueQdnWqRuBEgUCT793MtJ7PRWKNTYBdsEr7HJBb9d }, { index=2755972562 name=ZJoF	� addresses=[{ id=DdzFFzCqrhswhVECMd9DMAsgkKWwV3xvN6w6LDpCPd2jQnucuAypABZ5Bpto5RMJtF9SM9KRHrGnroWvZJM6koGLzTJp9EGNrQfYhA2m used=True changeAddress=False }] amount=2 coin(s) walletId=Ae2tdPwUPEZBgastrNueQdnWqRuBEgUCT793MtJ7PRWKNTYBdsEr7HJBb9d }]
[wallet.api:INFO:ThreadId 1921] [2018-08-31 09:36:40.85 UTC] 
GET Request #124
    :> api
    :> v1
    :> wallets
    :> 'walletId' field: Ae2tdPwUPEZBgastrNueQdnWqRuBEgUCT793MtJ7PRWKNTYBdsEr7HJBb9d
    :> accounts
    :> pagination: page=2, per_page=50
[wallet.api:INFO:ThreadId 1921] [2018-08-31 09:36:40.86 UTC] 
    Response #124 OK 0.00513576s > 
	status=success
	meta={ pagination=2/1 total=4 per_page=50 }
	data=[]
[wallet.api:INFO:ThreadId 1921] [2018-08-31 09:36:40.86 UTC] 
POST Request #125
    :> api
    :> v1
    :> transactions
    :> request body: { source={ walletId=Ae2tdPwUPEZBgastrNueQdnWqRuBEgUCT793MtJ7PRWKNTYBdsEr7HJBb9d accountIndex=2147483648 } destinations=[... (1 item(s))] groupingPolicty= spendingPassword= }
[legacyServantBackend:DEBUG:ThreadId 1929] [2018-08-31 09:36:40.91 UTC] sendMoney: processed addrs
[wallet.api:INFO:ThreadId 1921] [2018-08-31 09:36:40.91 UTC] 
    Response #125 RequestError "Cannot send transaction: Transaction creation error: not enough money, need 7500000052681 coin(s) more" 0.048222791s
RequestError "Cannot send transaction: Transaction creation error: not enough money, need 7500000052681 coin(s) more"```

Copy link
Contributor

@adinapoli-iohk adinapoli-iohk left a comment

Choose a reason for hiding this comment

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

Good job @paweljakubas ! This looks good apart from one minor thing which could inflate CI times a bit, and we definitely want to keep that under control 😉

After you amend, that's good to go on my and @edsko 's sides (we co-reviewed together).

@@ -98,6 +98,67 @@ accountSpecs wRef wc =

map (AccountBalance . accAmount) accsUpdated `shouldBe` balancesPartialUpdated



it "redeeming avvv key gives rise to the corresponding increase of balance of wallet'account - mnemonic not used" $ do
Copy link
Contributor

Choose a reason for hiding this comment

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

Little typo, it's "avvm".

Wallet{..} <- createWalletCheck wc newWallet

--adding new account
rAcc <- generate arbitrary :: IO NewAccount
Copy link
Contributor

Choose a reason for hiding this comment

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

For now this is OK -- but remember to fix it as part of CBR-408! 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

we have a new input - actually doing generate arbitrary in the body of property test is not what we want (long term). But I guess this is not place to do the refactorings. @KtorZ will probably organize a call this week to talk about this

Copy link
Contributor Author

@paweljakubas paweljakubas Sep 10, 2018

Choose a reason for hiding this comment

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

I already removed all generate/etc from integration tests here: https://github.com/input-output-hk/cardano-sl/compare/paweljakubas/CBR-408/improve-integration-tests and hope this one will be merged soon (after adding proper instances). And then return to this ticket, rebase and adopt pick instead of generate

-- state-demo/genesis-keys/keys-fakeavvm/fake-9.seed
let avvmKey = "QBYOctbb6fJT/dBDLwg4je+SAvEzEhRxA7wpLdEFhnY="
--password is set to Nothing
passPhrase <- generate (pure mempty) :: IO SpendingPassword
Copy link
Contributor

Choose a reason for hiding this comment

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

Another 👿 generate call 😉


txn <- fmap wrData etxn `shouldPrism` _Right

threadDelay 180000000
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to reduce the number of seconds you wait here -- this is inflating the CI running time of 3 minutes 😉

What about ~30/60 seconds? That should be plenty for the node to accept the block with the new transaction.

@paweljakubas paweljakubas force-pushed the paweljakubas/CBR-398/add-integration-tests-for-redeemADA branch from 27cd8e8 to 0c9a291 Compare September 3, 2018 10:08
@paweljakubas
Copy link
Contributor Author

paweljakubas commented Sep 3, 2018

Changes adopted:

rebased

  1. avvv -> avvm
  2. changed time to 90 seconds (I see 2-3 minutes in transaction related tests).
  3. resigned with passPhrase <- generate (pure mempty) :: IO SpendingPassword to
    passPhrase <- pure mempty :: IO SpendingPassword to my surprise the first version occasionally gave rise to

  integration/Util.hs:126:5: 
  1) Accounts redeeming avvm key gives rise to the corresponding increase of balance of wallet'account - mnemonic not used
       predicate failed on: Left (ClientWalletError (UnknownError "Request error (Passphrase doesn't match)"))```

@paweljakubas
Copy link
Contributor Author

paweljakubas commented Sep 3, 2018

walletIntegration error come from TransactionsSpecs.hs:

NotEnoughMoney 155423
[wallet.api:INFO:ThreadId 1711] [2018-09-03 10:44:11.39 UTC] 
GET Request #120
    :> api
    :> v1
    :> wallets
    :> 'walletId' field: Ae2tdPwUPEZK31U8P8MDBgQHz1TuyswXEKeZ9FdCZpbq6ugnde9gW98LrpL
    :> accounts
    :> pagination: page=1, per_page=50
[wallet.api:INFO:ThreadId 1711] [2018-09-03 10:44:11.40 UTC] 
    Response #120 OK 0.00528273s > 
	status=success
	meta={ pagination=1/1 total=5 per_page=50 }
	data=[{ index=2147483648 name=Initial account addresses=[{ id=DdzFFzCqrht3QDUMripMJJHYJEDcS6AmzhZV86Spzcq5hfem3gRXPJ15BEJsX8FiSeZPLZPEadcrrkGoPfM3yMuiA9ZARpZJGn2JWLSD used=True changeAddress=False }] amount=7837499923604 coin(s) walletId=Ae2tdPwUPEZK31U8P8MDBgQHz1TuyswXEKeZ9FdCZpbq6ugnde9gW98LrpL }, { index=2163940929 name=f"F==R򁭪j􁲔-�P󾸦C�㱗󖗞�	kQ𞞚 addresses=[{ id=DdzFFzCqrhsgDxQ1eyEYxKDdkP4zBMvDMVvwwWdVWptfhsJZVDyVwUhmHFcPQjqCPQ1kcGxB5PqAJJN8yeuaZKNXeTFZ2rWBBmqFrQcd used=True changeAddress=False }] amount=1 coin(s) walletId=Ae2tdPwUPEZK31U8P8MDBgQHz1TuyswXEKeZ9FdCZpbq6ugnde9gW98LrpL }, { index=2837889872 name=z�
o�𮄸m󩋚񠯉�7_ addresses=[{ id=DdzFFzCqrhsoxUX8X8aNPpNHzP2R4haJo319Pj3KxEA5L5jugdRSMSKe3FQR8GmWNsrcztVaT86a9k6Pgn3z8J15nABokhSgkdM9CDhE used=True changeAddress=False }] amount=2 coin(s) walletId=Ae2tdPwUPEZK31U8P8MDBgQHz1TuyswXEKeZ9FdCZpbq6ugnde9gW98LrpL }, { index=4213141939 name=hello addresses=[{ id=DdzFFzCqrht1xFKx3iKfuqM5uhthhhS84orzp57jwGMGwb2QtPSNUEi629Jpgne471y2s5gArzGYs21w6XmKo5YVZKx7raNss7AevpFY used=False changeAddress=False }, { id=DdzFFzCqrhtBtSmZRNFmu5ZzDENrWUzWLm8pVX32LpEy86FPNy1dvkUKfULuSaKJqnWLtkFhQxyYNBGC5GqYnePGE6n5iKYbsqX74UtB used=False changeAddress=False }] amount=0 coin(s) walletId=Ae2tdPwUPEZK31U8P8MDBgQHz1TuyswXEKeZ9FdCZpbq6ugnde9gW98LrpL }, { index=4244440456 name==򲈓,u3𨠬[
                                                                                                                                      )�-j瘼1񗅟4񆓸ug񈯖J0F addresses=[{ id=DdzFFzCqrhssLJAkpqY37a94cMc6NxTbnwq8XYbp9b7WzPrtZDewr7botqDMCbiewVF4pmPhWYLAGm5Fuoi3UwVzVcavaCQU4HQt7DE1 used=True changeAddress=False }] amount=5 coin(s) walletId=Ae2tdPwUPEZK31U8P8MDBgQHz1TuyswXEKeZ9FdCZpbq6ugnde9gW98LrpL }]
[wallet.api:INFO:ThreadId 1711] [2018-09-03 10:44:11.40 UTC] 
GET Request #121
    :> api
    :> v1
    :> wallets
    :> 'walletId' field: Ae2tdPwUPEZK31U8P8MDBgQHz1TuyswXEKeZ9FdCZpbq6ugnde9gW98LrpL
    :> accounts
    :> pagination: page=2, per_page=50
[wallet.api:INFO:ThreadId 1711] [2018-09-03 10:44:11.40 UTC] 
    Response #121 OK 0.00514951s > 
	status=success
	meta={ pagination=2/1 total=5 per_page=50 }
	data=[]
[wallet.api:INFO:ThreadId 1711] [2018-09-03 10:44:11.40 UTC] 
POST Request #122
    :> api
    :> v1
    :> transactions
    :> request body: { source={ walletId=Ae2tdPwUPEZK31U8P8MDBgQHz1TuyswXEKeZ9FdCZpbq6ugnde9gW98LrpL accountIndex=2147483648 } destinations=[... (1 item(s))] groupingPolicty= spendingPassword= }
[legacyServantBackend:DEBUG:ThreadId 1719] [2018-09-03 10:44:11.45 UTC] sendMoney: processed addrs
[wallet.api:INFO:ThreadId 1711] [2018-09-03 10:44:11.45 UTC] 
    Response #122 RequestError "Cannot send transaction: Transaction creation error: not enough money, need 15675000002589 coin(s) more" 0.047372323s

@adinapoli-iohk
Copy link
Contributor

@paweljakubas The reason why you had to change your generator for SpendingPassword is quite simple: if you take a look at how randomWallet is implemented, you will see that it never sets a spending password:

randomWallet :: WalletOperation -> IO NewWallet
randomWallet walletOp =
    generate $
        NewWallet
            <$> arbitrary
            <*> pure Nothing
            <*> arbitrary
            <*> pure "Wallet"
            <*> pure walletOp

And the occasion failures you saw were probably the ones where generate arbitrary was generating a spending password which was not mempty.

I think what you did here is fine, but you should document the above in your code, to make sure the reader understands why you are passing an empty spending password. Of course there is the open question whether or not randomWallet should be parametrised on a spendingPassword, but that's for a separate discussion :)

@paweljakubas paweljakubas force-pushed the paweljakubas/CBR-398/add-integration-tests-for-redeemADA branch from 0c9a291 to 33c3970 Compare September 3, 2018 19:23
erikd
erikd previously requested changes Sep 5, 2018
Copy link
Member

@erikd erikd left a comment

Choose a reason for hiding this comment

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

The wallet integration tests are failing on this, but its not the know flakey test that failing but the determinisic test. Specifically in https://hydra.iohk.io/build/228408/log

  integration/TransactionSpecs.hs:103:13: 
  1) Transactions asset-locked wallets can receive funds and transactions are confirmed in index
       not expected: 0

  integration/TransactionSpecs.hs:133:17: 
  2) Transactions sending from asset-locked address in wallet with no ther addresses gets 0 confirmations from core nodes
       uncaught exception: PatternMatchFail
       integration/TransactionSpecs.hs:133:17-74: Irrefutable pattern failed for pattern txnEntry : _

The flakey parts of that test are being disabled which will make it easier to see this problem.

@erikd
Copy link
Member

erikd commented Sep 5, 2018

The flakey test has been removed from the wallet integration test. Please rebase against current develop. If you still have a failing test, then something got inadvertently broken.

@erikd erikd dismissed their stale review September 5, 2018 06:37

See comment

@paweljakubas paweljakubas force-pushed the paweljakubas/CBR-398/add-integration-tests-for-redeemADA branch from 33c3970 to 685aaf8 Compare September 5, 2018 07:04
@paweljakubas
Copy link
Contributor Author

I rebased and it seems now integration tests with redeemADA test added have passed

@paweljakubas paweljakubas force-pushed the paweljakubas/CBR-398/add-integration-tests-for-redeemADA branch from 685aaf8 to 65cf63b Compare September 5, 2018 09:57
@paweljakubas
Copy link
Contributor Author

Rebased once again against develop

@paweljakubas
Copy link
Contributor Author

Accounts
  can retrieve only an account's balance
  can retrieve only an account's addresses
  can retrieve initial and updated balances of several accounts from getAccountBalances that are equivalent to what is obtained from getAccounts
  redeeming avvm key gives rise to the corresponding increase of balance of wallet'account - mnemonic not used
Addresses
  Creating an address makes it available
  Index returns real data
Wallets
  Creating a wallet makes it available.
  Updating a wallet persists the update
  CreateWallet with the same mnemonics rises WalletAlreadyExists error
  RestoreWallet with the same mnemonics throws WalletAlreadyExists
  Can accept Unicode characters
  creating wallet gives rise to an empty Utxo histogram
Transactions
[TEST-LOG] Account {accIndex = AccountIndex {getAccIndex = 2147483648}, 
...

1536143941307925, txStatus = Persisted}
  asset-locked wallets can receive funds and transactions are confirmed in index FAILED [1]
  sending from asset-locked address in wallet with no ther addresses gets 0 confirmations from core nodes FAILED [2]
  estimate fees of a well-formed transaction
  fails if you spend too much money
  posted transactions gives rise to nonempty Utxo histogram
    # PENDING: No reason given
Servant API Properties
  V0 API follows best practices & is RESTful abiding
  V1 API follows best practices & is RESTful abiding

Failures:

  integration/TransactionSpecs.hs:103:13: 
  1) Transactions asset-locked wallets can receive funds and transactions are confirmed in index
       not expected: 0

  integration/TransactionSpecs.hs:133:17: 
  2) Transactions sending from asset-locked address in wallet with no ther addresses gets 0 confirmations from core nodes
       uncaught exception: PatternMatchFail
       integration/TransactionSpecs.hs:133:17-74: Irrefutable pattern failed for pattern txnEntry : _

Randomized with seed 47286649```

@paweljakubas
Copy link
Contributor Author

paweljakubas commented Sep 5, 2018

The test I added passed (it is in Accounts spec last position). Unfortunately, at present the more tests we have, the more fragmented the coins distribution becomes, and thus, the more likely overall the tests are to fail.

@erikd
Copy link
Member

erikd commented Sep 5, 2018

@paweljakubas That suggests we need to split the tests up so that every test begins in a state such that the test will only fail if there actually is something wrong.

@paweljakubas
Copy link
Contributor Author

@erikd I talked about this with @KtorZ and our squad plans to provide the comprehensive solution to that in coming weeks. If this happens, the redeemADA test will be added to integration test suite I suppose

@paweljakubas paweljakubas force-pushed the paweljakubas/CBR-398/add-integration-tests-for-redeemADA branch from 65cf63b to 0dc9013 Compare September 10, 2018 09:16
@paweljakubas
Copy link
Contributor Author

paweljakubas commented Sep 10, 2018

rebased, and used prop/withSuccesses/pick/monadicIO - removed generate occurences (only for redeemADA test)

Copy link
Contributor

@KtorZ KtorZ left a comment

Choose a reason for hiding this comment

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

A few comments, mostly cosmetic.
I'll have that merged however because it works fine, but please @paweljakubas have a look at them for the record 😄

balancePartialRespB <- run $ getAccountBalance wc walId (accIndex newAcct)
balancesPartialB <- run $ wrData <$> balancePartialRespB `shouldPrism` _Right
let zeroBalance = AccountBalance $ V1 (Core.mkCoin 0)
liftIO $ balancesPartialB `shouldBe` zeroBalance
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unnecessary. Since newAcct has already been fetched, you can check the balance of this one should you have to.


--password is set to Nothing in the current implementation of randomWallet
--when it changes redemptionSpendingPassword handles it, otherwise passPhare addresses it
passPhrase <- pure mempty :: PropertyM IO SpendingPassword
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor note:

You can use TypeApplication or apply the type annotation to the mempty directly to avoid mentioning all the context around pure (mempty @SpendingPassword) or pure (mempty :: SpendingPassword)

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, see note below.

, redemptionMnemonic = Nothing
, redemptionSpendingPassword = case newwalSpendingPassword newWallet of
Just spPassw -> spPassw
Nothing -> passPhrase
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor note:

Reads better as:

   let redemption = Redemption
                    { redemptionRedemptionCode = ShieldedRedemptionCode avvmKey
                    , redemptionMnemonic = Nothing
                    , redemptionSpendingPassword = fromMaybe mempty (newwalSpendingPassword newWallet)

liftIO $ balancesPartial `shouldBe` nonzeroBalance

--redeemAda for the same redeem address should result in error
etxnAgain <- run $ redeemAda wc redemption
Copy link
Contributor

Choose a reason for hiding this comment

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

Would have been nice to have a separated test for that as we're testing a different requirement with this 👍

Copy link
Contributor

@adinapoli-iohk adinapoli-iohk left a comment

Choose a reason for hiding this comment

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

LGTM, I think at this stage get going with the integration tests is more important than getting trapped into the details. We can give this one a second look once we start overhauling the integration tests for good.

@KtorZ KtorZ merged commit 6327379 into develop Sep 10, 2018
@KtorZ KtorZ deleted the paweljakubas/CBR-398/add-integration-tests-for-redeemADA branch September 13, 2018 04:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants