[CBR-390] Fixes TxMeta fields and tests #3473
Conversation
| , _txMetaIsLocal = False | ||
| , _txMetaIsOutgoing = False | ||
| , _txMetaIsLocal = allOurs | ||
| , _txMetaIsOutgoing = outCoin < inCoin |
There was a problem hiding this comment.
I find this definition very confusing. At first glance it looks like this says: "a transaction is considered outgoing if there's less going out than coming in". It doesn't say that, of course, and I do think the definition is right, but I had to read it 10 times before I was convinced. I think it's confusing because inCoin suggests this is "incoming" -- but that's actually the outgoing part ("in" not for "incoming" but for "input"), and outCoin suggests this is "outgoing" -- but is actually the incoming part ("out" not for "outgoing" but rather for "output"). I would suggest renaming. Perhaps reducedAccBalance and increasedAccBalance or something like that? Not sure what the best names are, but I think as is it's just too confusing.
There was a problem hiding this comment.
you're right, I rename them to spentInputCoins and gainedOutputsCoins.
| @@ -38,6 +41,9 @@ import Pos.Wallet.Web.Tracking.Decrypt (WalletDecrCredentialsKey (..), | |||
| Submit pending transactions | |||
| -------------------------------------------------------------------------------} | |||
|
|
|||
|
|
|||
| type PartialTxMeta = Bool -> Coin -> IO TxMeta | |||
There was a problem hiding this comment.
This needs a comment explaining what it's for and what the arguments are. Does it really need to live in IO?
| metaForNewTx :: Monad m => Core.Timestamp -> | ||
| HdAccountId -> TxId -> NonEmpty (TxIn, TxOutAux) -> NonEmpty TxOut -> Bool -> Coin -> Bool -> Coin -> m TxMeta | ||
| metaForNewTx time accountId txId inputs outputs allInpOurs inCoin allOutOurs outCoin = | ||
| return $ TxMeta { |
There was a problem hiding this comment.
This duplicates the logic from above, it would be nice if they shared some code, especially because some of these definitions are a bit confusing.
| metas = map (\acc -> resolvedToTxMeta tx | ||
| (nothingToZero acc prefInCoins) | ||
| (nothingToZero acc prefOutCoins) | ||
| (onlyOurInps && onlyOurOuts) acc) allAccounts | ||
|
|
||
| -- | Prefilter inputs of a transaction |
There was a problem hiding this comment.
I think this whole prefiltering code needs a tidying pass, comments added, naming changed, etc. but we can take a look at that another time.
|
Going to merge this as-is because we need the results in the wallet restoration work. @kderme will submit a follow up PR addressing the above. |
[CBR-390] Fixes TxMeta fields and tests
…hk/kderme/CBR-390 [CBR-390] Fixes TxMeta fields and tests
Description
This fixes all TxMeta field and adds tests.
Linked issue
CBR-390
Type of change
Developer checklist
Testing checklist
QA Steps
Screenshots (if available)