[Wallet] Fix transaction history by fassadlr · Pull Request #548 · stratisproject/StratisFullNode

fassadlr · 2021-05-05T13:07:13Z

Tests still need to be fixed.

quantumagi · 2021-05-06T04:09:53Z

src/Stratis.Bitcoin.Features.Wallet/Interfaces/IWalletManager.cs

+        /// <param name="offset">Skip (offset) the result set by this amount (primarily used for pagination).</param>
        /// <returns>Collection of address history and transaction pairs.</returns>
-        IEnumerable<AccountHistory> GetHistory(string walletName, string accountName = null, long? prevOutputTxTime = null, int? prevOutputIndex = null, int? take = int.MaxValue, string searchQuery = null);
+        IEnumerable<AccountHistory> GetHistory(string walletName, string accountName = null, string searchQuery = null, int? limit = int.MaxValue, int? offset = 0);


Offsets, although often used, are generally not 100% reliable due to the fact that the database can change between subsequent queries. In this case, with chronologically ordered data (only the receipts, not the spends), its probably ok and should still work 99.99% of the time assuming rewinds/forks are not occurring all that frequently. Issues, if they occur, will be in the form of omitted or duplicated rows at paging boundaries.

Yeah, I'm not too concerned this. We have to remember that the history call is currently only used by the UI for the user's benefit. Unless this logic someday gets used for business critical results, I wouldn't worry about it too much.

quantumagi · 2021-05-06T04:23:34Z

src/Stratis.Bitcoin.Features.Wallet/Interfaces/IWalletRepository.cs

        /// </summary>
-        /// <param name="walletName">The name of the wallet to return the transactions of.</param>
-        /// <param name="accountName">An optional account name to limit the results to a particular account.</param>
+        /// <param name="account">An optional account name to limit the results to a particular account.</param>


How are the results limited to a specific wallet? The database contains the data for multiple wallets so it must be possible to specify a wallet in the absence of a specific wallet account being specified. This could be why the walletName was a separate parameter in the legacy implementation.

We get the wallet name from the account that is passed in's account root. This is called from:

https://github.com/stratisproject/StratisFullNode/pull/548/files#diff-481c41c671b79c78e1987b366a78efdd29721cb50dfff8bffe8881a830da91e3R847

Is this OK?

We won't have the wallet name if the account is passed as null here.

quantumagi · 2021-05-06T04:37:53Z

src/Stratis.Bitcoin.Features.Wallet/WalletManager.cs

                foreach (HdAccount account in accounts)
                {
-                    accountsHistory.Add(this.GetHistoryForAccount(account, prevOutputTxTime, prevOutputIndex, take.GetValueOrDefault(), searchQuery));
+                    accountsHistory.Add(this.GetHistoryForAccount(account, limit.GetValueOrDefault(), 0, searchQuery));


The offset is not passed. Can the offset be used in this way - i.e. across accounts? I suppose the alternative is to use a single query (instead of multiple calls to retrieve each account's info individually)?

Ah this was a mistake on my part. I will fix this.

quantumagi · 2021-05-06T04:42:48Z

src/Stratis.Features.SQLiteWalletRepository/Tables/HDTransactionData.cs

+                    GROUP BY 
+                        t.OutputTxId
+                    ORDER BY
+                        t.OutputTxTime DESC


Ordering is happening by the "receiving time" of the UTXO even when its being spent/sent at a different time. Should the data be merged with a query getting all the "sends" ordered separately - i.e. using SpendTxTime?

@quantumagi this is very strange as the SQL i get back is perfectly ordered... I.e receives/sends/stakes/mines all in perfect date order? 🤔

It seems we will only be displaying the receive date - so that alone would be an issue, and then secondly we only order by the original receive date rather than using the spent date when applicable to the transaction type.

fassadlr · 2021-05-06T15:19:36Z

...ratis.Bitcoin.Features.SmartContracts.Tests/Controllers/SmartContractWalletControllerTest.cs

-            Assert.Equal(new uint256(toSkip), model.ElementAt(toTake - 1).Hash);
-            Assert.Equal(new uint256(toSkip + toTake - 1), model.ElementAt(0).Hash);
-        }
+        //[Fact]


I'm still busy fixing/porting these.

fassadlr · 2021-05-06T15:19:48Z

src/Stratis.Bitcoin.Features.SmartContracts/Wallet/SmartContractWalletController.cs

                AccountHistory accountHistory = accountsHistory.First();

-                List<FlatHistory> items = accountHistory.History.Where(x => x.Address.Address == request.Address).ToList();
+                List<FlatHistory> items = new List<FlatHistory>();// accountHistory.History.ToList();//.Where(x => x.Address.Address == request.Address).ToList();


I'm still busy fixing this.

fassadlr · 2021-05-06T15:20:33Z

src/Stratis.Bitcoin.Features.Wallet.Tests/WalletControllerTest.cs

        }

        [Fact]
-        public async Task GetHistoryWithoutAddressesReturnsEmptyModel()


All these tests I'm moving to one encompassing integration test as we can't mock results any longer due to us query the DB directly with SQL.

fassadlr · 2021-05-06T15:21:12Z

src/Stratis.Bitcoin.Features.Wallet/Controllers/WalletController.cs

                async (req, token) => this.Json(await this.walletService.Consolidate(req, token)));
        }
-
-        private TransactionItemModel FindSimilarReceivedTransactionOutput(List<TransactionItemModel> items,


fassadlr · 2021-05-06T15:22:02Z

src/Stratis.Bitcoin.IntegrationTests/Wallet/WalletTests.cs

-        /// Copies the test wallet into data folder for node if it isn't already present.
-        /// </summary>
-        /// <param name="path">The path of the folder to move the wallet to.</param>
-        private void InitializeTestWallet(string path)


fassadlr · 2021-05-06T15:22:18Z

src/Stratis.Features.SQLiteWalletRepository.Tests/WalletRepositoryTests.cs

-                }
-            }
-        }
+        //[Fact]


Still busy fixing...

fassadlr · 2021-05-10T05:57:10Z

Hi @quantumagi, I'm keen for this PR to go in as is 😎 as I don't want to bloat it more due to the amount of changes already in it. Do you have any more comments on my replies? I'm pretty confident that the integration test I wrote covers a lot of bases

There are still tests commented out but they relate to queries where we search/filter by address, especially in smart contracts. The smart contracts controller's get history method and related tests relies on the address "filter", so after this PR is merged I want to get on to fixing/reimplementing those.

What are your thoughts?

quantumagi · 2021-05-10T06:18:36Z

@fassadlr , the main issue i can see is spends/receives not ordered correctly by also utilising the spend date. I think if that is fixed and there are no other bigger issues we should be good.

fassadlr · 2021-05-13T10:00:40Z

@quantumagi as dicussed, if the CI passes we should merge this, so that I can then look at the address filtering.

* WIP * Single SQL query * Refactor * WIP * Tests still need to be fixed * Self review * Self review * Fixing tests and review changes * Fix Test * Fix Test * busy with test * WIP * SQL changes based on review * Update WalletHistoryIntegrationTests.cs * Fix test * Fix Tests * Review fixes

fassadlr added 7 commits May 4, 2021 12:31

WIP

35ce1fa

Single SQL query

ac55923

Refactor

95040e3

WIP

972c551

Tests still need to be fixed

91f74f4

Self review

fccf280

Self review

9352627

quantumagi reviewed May 6, 2021

View reviewed changes

Fixing tests and review changes

fd4042a

fassadlr commented May 6, 2021

View reviewed changes

src/Stratis.Features.SQLiteWalletRepository.Tests/WalletRepositoryTests.cs

}

}

}

//[Fact]

Copy link

Contributor Author

fassadlr May 6, 2021

Choose a reason for hiding this comment

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

Still busy fixing...

fassadlr added 3 commits May 7, 2021 08:14

Merge remote-tracking branch 'upstream/master' into fix-history

d592311

Fix Test

59a9750

Fix Test

be56bc5

fassadlr added 7 commits May 10, 2021 08:09

busy with test

7aaa6bf

WIP

65ffcbe

SQL changes based on review

a65a639

Update WalletHistoryIntegrationTests.cs

bc1bc2e

Fix test

1c2d9b4

Fix Tests

8e3447e

Review fixes

3123c67

fassadlr changed the title ~~[Wallet] Fix transaction history (WIP)~~ [Wallet] Fix transaction history May 13, 2021

fassadlr merged commit e16c8f0 into stratisproject:master May 13, 2021

fassadlr deleted the fix-history branch May 13, 2021 11:45

Conversation

fassadlr commented May 5, 2021

Uh oh!

quantumagi May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quantumagi May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fassadlr May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quantumagi May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quantumagi May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quantumagi May 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fassadlr May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fassadlr commented May 10, 2021

Uh oh!

quantumagi commented May 10, 2021

Uh oh!

fassadlr commented May 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

quantumagi May 6, 2021 •

edited

Loading

quantumagi May 6, 2021 •

edited

Loading

fassadlr May 6, 2021 •

edited

Loading

quantumagi May 6, 2021 •

edited

Loading

quantumagi May 6, 2021 •

edited

Loading

quantumagi May 10, 2021 •

edited

Loading

fassadlr May 6, 2021 •

edited

Loading