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

[CO-432] Add foreign transaction to input and remove updateForeign #3768

Merged
merged 1 commit into from
Nov 14, 2018

Conversation

paweljakubas
Copy link
Contributor

@paweljakubas paweljakubas commented Oct 18, 2018

Description

At this moment prefiltering comes down to filtering out the relevant inputs and so the regular call to updatePending fails to see that the foreign transaction that got removed. Up till now we were looking at the outputs instead of the inputs not because of any fundamental difference between regular and foreign pending transactions, but rather because prefiltering is removing some information that we need and so we look at the outputs as a (somewhat poor) substitute instead. As a consequence we have updatePending and updateForeign removing corresponding inputs and outputs, respectively.

Here, the following changes are proposed:

  1. We adopt lookup seeking one of our currently tracked foreign transaction to appear in the block's inputs, and if so, make sure it appears in the prefiltered block
    (a) getters for foreign pending block are added
    (b) in prefilterTxForWallets we search inputs for transactions that have the same id as the foreign transation in the respective pending set
    (c) PrefilteredBlock is extended by pfbForeignInputs and populated if search of point 1b is successful
    (d) refactoring of updatePending in such a way that it works also with pfbForeignInputs and service both input and foreign input checkpoint
    (e) update of both (wallet-new/src/Cardano/Wallet/Kernel/DB/Spec/Update.hs) - applyBlock and applyBlockPartial is done in the context of the change of point 1d
  2. updateForeign is removed
  3. updateUtxo is changed in such a way that it includes rather (Set.union pfbInputs pfbForeignInputs) than pfbInputs

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CO-432

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

$ ./scripts/build/cardano-sl.sh wallet-new
$ nix-build -A walletIntegrationTests -o launch_integration_tests --arg useStackBinaries true
$ ./launch_integration_tests

@paweljakubas
Copy link
Contributor Author

As CI stucked on a number of jobs, I squashed the commits, rebased and force pushed

@paweljakubas
Copy link
Contributor Author

paweljakubas commented Oct 19, 2018

concerning integration tests: PR affects how foreign transaction is dealt and we have:

  redeeming avvm key gives rise to the corresponding increase of balance of wallet'account - mnemonic not used
    +++ OK, passed 1 tests.

The failed test:

Failures:

  integration/TransactionSpecs.hs:102:22: 
  1) Transactions asset-locked wallets can receive funds and transactions are confirmed in index
       Falsifiable (after 1 test):
       
       not expected: 0

was present irrespective of my changes (I also checked this on my computer and corroborate this statement)

@paweljakubas paweljakubas force-pushed the paweljakubas/CO-432/change-prefilterTxForWallets branch from 30dc141 to 20ddb13 Compare October 30, 2018 08:40
@paweljakubas paweljakubas force-pushed the paweljakubas/CO-432/change-prefilterTxForWallets branch from 20ddb13 to c93e8b8 Compare October 30, 2018 09:44
@paweljakubas
Copy link
Contributor Author

paweljakubas commented Oct 30, 2018

At this moment it is double counting in an account balance - I tried also with

utxoMin   = utxoUnion `Core.utxoRestrictToInputs` (Set.union pfbInputs pfbForeignInputs)
utxo'     = utxoUnion `Core.utxoRemoveInputs`     (Set.union pfbInputs pfbForeignInputs)

in updateUtxo PrefilteredBlock{..} (utxo, balance) with the same result

@paweljakubas paweljakubas force-pushed the paweljakubas/CO-432/change-prefilterTxForWallets branch 2 times, most recently from c8c22b4 to 809c457 Compare October 31, 2018 12:08
@paweljakubas
Copy link
Contributor Author

Added Ryan's suggestions, rebased and squashed into one commit

@paweljakubas paweljakubas force-pushed the paweljakubas/CO-432/change-prefilterTxForWallets branch 2 times, most recently from a23e48c to 5d3a71a Compare November 5, 2018 15:30
@paweljakubas
Copy link
Contributor Author

paweljakubas commented Nov 5, 2018

rebased and adopted a number of simplifications : (a) get rid of foldr in favour of maps and (b) moving reindexedforeignInput upstream

@paweljakubas paweljakubas force-pushed the paweljakubas/CO-432/change-prefilterTxForWallets branch 5 times, most recently from 5757f6c to e25f858 Compare November 9, 2018 10:14
[CO-432] fix applyBlockPartial

[CO-432] fixing lookup and updateUtxo

[CO-432] accommodating Ryan's suggestions

[CO-432] Get rid of foldr in favour of maps and moving reindexing upstream

[CO-432] hlint calming

[CO-432] Further code cleaning
@paweljakubas
Copy link
Contributor Author

PR moved to cardano-wallet and hence closed here

@KtorZ KtorZ reopened this Nov 14, 2018
@KtorZ
Copy link
Contributor

KtorZ commented Nov 14, 2018

Re-opening here, we still need to maintain both repository in sync during the transition (i.e. until cardano-wallet becomes a submodule of cardano-sl)

@KtorZ KtorZ merged commit ad7dc40 into develop Nov 14, 2018
@KtorZ KtorZ deleted the paweljakubas/CO-432/change-prefilterTxForWallets branch November 14, 2018 12:15
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.

2 participants