Fix for the listAssets API which shows assets unrelated to a wallet

[Unisay]

Issue Number

ADP-710

Problematic current state:

list of wallet assets WAs derived like this:

for each transaction T in blockchain:
if T has at least one output associated with the current wallet:
add T to the list of Wallet transactions WTs;

for each transaction T in WTs:
for each output O of T:
for each asset A in token bundle of O:
add A to the list of wallet assets WAs;

Solution №1 (unfoldable, superseded by the Solution №2, see below)

When restoring wallet state from blocks of transactions all transaction outputs that are not related to wallet address are omitted;

for each transaction T in blockchain:
if T has at least one output associated with the current wallet:
let T' be T with all unrelated* outputs removed;
add T' to the list of wallet transactions WTs;

for each transaction T in WTs:
for each output O of T:
for each asset A in token bundle of O:
add A to the list of wallet assets WAs;

• output is considered to be unrelated if its address if not the one belonging to the wallet;

Consequences

• Wallet states collected before the fix contain transactions with all outputs (including unrelated) In order for the unrelated assets (those found in unrelated outputs) to disappear wallet state needs to be restored from scratch.

• Modified transactions T' are no longer valid as some of their outputs were omitted;

Solution №2 (unfoldable, superseded by the Solution №3, see below)

Solution №2

for each transaction T in blockchain:
if T has at least one output associated with the current wallet:
let T' be T with all outputs tagged with their belonging (ours/theirs) - b(O);
add T' to the list of wallet transactions WTs;

for each transaction T in WTs:
for each output O of T:
if b(O) is not Ours then skip this O;
for each asset A in token bundle of O:
add A to the list of wallet assets WAs;

Solution №3

for each transaction T in WTs:
for each output O of T:
if O is not Ours then skip this O;
for each asset A in token bundle of O:
add A to the list of wallet assets WAs;

Ad-hoc improvements

• Move asset extraction logic to the Cardano.Wallet module
• Add an extra property test to cover transaction application.

[Unisay]

@rvl

I implemented what we discussed today: instead of filtering "foreign" assets at "query" time we instead filter them at wallet restore time, omitting "foreign" TX outputs.

Please take a look 👀

[rvl]

Great

[rvl]

Yes this looks like it will work for listAssets.
If everything breaks due to this change, then perhaps we could annotate the TxOut address about whether it belongs to the wallet.

[HeinrichApfelmus]

Uhh, this does indeed look like it could break everything. 😅 I think the original idea for the Tx type was to represent a transaction exactly as it appears on the chain — no modifications. (I believe that the adjustment to the fee field is a hack, as this field did not exist in Byron, but is added for clarity.) Changing this assumption has several implications:

• The /v2/wallets/{walletId}/transactions endpoint changes in a backwards-incompatible way and no longer presents transactions as they appear on-chain. For example, cardano-node will return a different result than this endpoint if you give it the same transaction hash.
• Tx are stored in the database via putTxHistory. If we change the contents of Tx, we would have to add a migration for the database as well.
• The inputIx field of the TxIn refers to an index in the list outputs. If we filter the list, the indices get rearranged. 😱 We are safe in that ourNextUTxO uses the original tx, not the updatedTx to compute the UTxO set. Users of the /v2/…/transactions endpoint are not so lucky. If we really want to filter outputs from the Tx, we should at least record their indices as well.

In light of these implications, my preference would be to keep the original idea of Tx representing a complete transaction, and do the filtering in listAssets a posteriori.

[rvl]

The Tx type in cardano-wallet has never been a transaction exactly as it appears on the chain. Multiple fields are missing!
It can be whatever we want or need it to be. We can compute info about the transaction once during restoration, then cache it in the db. An example of such info is the transaction amount.

I ask, do users really need to see TxOuts which don't concern their wallet?
If they do, perhaps the wallet should store the original serialized transaction in the database for this purpose.

Although, if our code is making assumptions all over the place that the TxOuts aren't filtered -- you raise a very good point about numbering -- then it's probably easiest to not filter, but instead annotate the TxOut address with a flag indicating whether the address belongs to this wallet.

Yes this will probably need database migrations; we can do migrations.
Also users will not see a propertly filtered listAssets unless they restore from genesis. This is also fine.

[Unisay]

@HeinrichApfelmus appreciate your input!
I've added Solution #2 to the issue description that takes your input into account as described by @rvl

[Unisay]

@HeinrichApfelmus finally I finished covering discussed implementation with property tests. Please take another look and if there are no obstacles - I'll merge it.

[HeinrichApfelmus]

🤔? I don't think that the hint is correct. "Collateral" has a very specific meaning and the UTxO can shrink even when no "collateral" inputs are used. Also, a transaction cannot be "foreign" if it spends an output belonging to the wallet!

I think the underlying issue is that there are two ways to recognize whether a transaction creates or spends an output belonging to our wallet: a) checking whether the address of a transaction output belongs to us and b) looking up a transaction input in our UTxO. (isOurTxgives a complete list of ways). In particular, transaction inputs cannot be recognized by their address — transaction inputs are stored as references and are not resolved. The only way to recognize a TxIn is to have a current UTxO set as reference — this is a subtle, but important point. In turn, this implies that the utxo arguments from all the tests are, in fact, transaction outputs that have once been recognized as "ours".

In that light, I recommend to remove the prop_applyOurTxToUTxO_noneOurs test — the assumption that no outputs ever belong to the test wallet logically implies that the argument utxo should empty. The logic that the test was trying to cover is probably already covered by prop_applyOurTxToUTxO_someOurs.