WalletFilterProcessor: Minor cleanup
#12126
Conversation
| /// <summary> | ||
| /// Used by test to emulate database. | ||
| /// </summary> | ||
| internal void AddToCache(IEnumerable<FilterModel> filters) | ||
| { | ||
| foreach (var filter in filters) | ||
| { | ||
| FiltersCache[filter.Header.Height] = filter; | ||
| } | ||
| } |
| private async Task<FilterModel> UpdateFiltersCacheAndReturnFirstAsync(uint startingHeight, CancellationToken cancellationToken) | ||
| { | ||
| FiltersCache.Clear(); | ||
| var filtersBatch = await BitcoinStore.IndexStore.FetchBatchAsync(new Height(startingHeight), MaxNumberFiltersInMemory, cancellationToken).ConfigureAwait(false); | ||
| foreach (var filter in filtersBatch) | ||
| { | ||
| FiltersCache[filter.Header.Height] = filter; | ||
| } | ||
|
|
||
| return filtersBatch.First(); | ||
| } |
There was a problem hiding this comment.
Inlined above. For some reason it feels better to me. Maybe it's because the name of the method does not help me too much in reading the code.
| /// <summary> | ||
| /// Return the keys to test against the filter depending on the height of the filter and the type of synchronization. | ||
| /// </summary> | ||
| /// <param name="filterHeight">Height of the filter that needs to be tested.</param> | ||
| /// <param name="syncType">First sync of TurboSync, second one, or complete synchronization.</param> | ||
| /// <returns>Keys to test against this filter</returns> | ||
| /// <seealso href="https://github.com/zkSNACKs/WalletWasabi/issues/10219">TurboSync specification.</seealso> |
There was a problem hiding this comment.
Moved up. This is actually quite important and there is a bigger chance that it's read if it is on the "class level".
| /// <remarks>Guards <see cref="SynchronizationRequests"/>.</remarks> | ||
| private object SynchronizationRequestsLock { get; } = new(); |
There was a problem hiding this comment.
We have just one lock here so Lock is the name that we use quite often in such situations.
There was a problem hiding this comment.
Yes, nice rename. The reason is that when designing this class we had 2 locks but we removed one
| } | ||
| } | ||
| /// <remarks>Internal only to allow modifications in tests.</remarks> | ||
| internal Dictionary<uint, FilterModel> FiltersCache { get; } = new(); |
There was a problem hiding this comment.
We only use internal that way, so the comment is useless. Feel free to keep it if you think it's right.
There was a problem hiding this comment.
I like to stress it because then refactorings are easier but you are as well.
| @@ -93,12 +96,11 @@ protected override async Task ExecuteAsync(CancellationToken cancellationToken) | |||
| { | |||
| while (true) | |||
| { | |||
| cancellationToken.ThrowIfCancellationRequested(); | |||
There was a problem hiding this comment.
@kiminuo This is the only change that I do not understand, can you explain it please?
There was a problem hiding this comment.
It's removed because the next line accepts cancellation token as well. So I don't see a reason for having this line.
kiminuo
deleted the
feature/2023-12-20-WalletFilterProcessor-minor-cleanup
branch
No description provided.