Use indexation cache to satisfy "coins to spend" queries #2463
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…to_spend_cache_part_2
…to_spend_cache_part_2
netrome
reviewed
Dec 16, 2024
There was a problem hiding this comment.
Nice stuff so far. I have only done a relatively shallow sweep and will get back again during the week for a deeper look. I have a few nits and suggestions so far, but nothing major.
I'm not a fan of the lack of explicit error handling in crates/fuel-core/src/graphql_api/storage/coins.rs but other than that I think the code looks generally really well-written.
…to_spend_cache_part_2
rafal-ch
added a commit
that referenced
this pull request
Jan 2, 2025
Closes #2474 ## Description This PR changes the following: 1. Adds new test in `tests/tests/relayer.rs` - to ensure that only **non**-retryable messages contribute to the balance returned from `balance()`, `balances()` and `coins_to_spend()`[^1] endpoints 2. Modifies the `balance()` test in `tests/tests/balances.rs` to also include some retryable message which is expected **not** to contribute to the total balance. * for `balances()` and `coins_to_spend()` it was already the case 1<sup>st</sup> test checks the behavior of the actual blockchain operations while the 2<sup>nd</sup> tests the proper initialization of the balances index at genesis. ## Checklist - [X] Breaking changes are clearly marked as such in the PR description and changelog - [X] New behavior is reflected in tests ### Before requesting review - [X] I have reviewed the code myself [^1]: currently, `coins_to_spend()` is not using indexation, this will change when [this PR](#2463) is merged. --------- Co-authored-by: Green Baneling <[email protected]>
1 task
xgreenx
reviewed
Jan 2, 2025
| excluded_ids: &ExcludedCoinIds<'_>, | ||
| batch_size: usize, | ||
| ) -> Result<Vec<CoinsToSpendIndexEntry>, CoinsQueryError> { | ||
| const TOTAL_AMOUNT_ADJUSTMENT_FACTOR: u64 = 2; |
There was a problem hiding this comment.
It would be nice to put a comment why we decided to do it like this=)
Comment on lines
+229
to
+235
| // TODO[RC]: No clone() required for coins? Double check the types used, | ||
| // maybe we want `MessageCoin` (which is Copy) for messages? | ||
| // 1) Currently we use the same types as embedded in the executor `Event`. | ||
| // 2) `MessageCoin` will refuse to construct itself from a `Message` if the data is empty | ||
| // impl TryFrom<Message> for MessageCoin { ... if !data.is_empty() ... } | ||
| // Actually it shouldn't matter from the indexation perspective, as we just need | ||
| // to read data from the type and don't care about which data type we took it from. |
There was a problem hiding this comment.
It is okay to use clone in tests=) I don't think that we need to add more specific type
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #2391
This PR includes all changes from the Part 1 PR, making it deprecated.
Description
Changes in this PR:
The new
CoinsToSpendindexMessagesorCoinson-chain databasesNonceUtxoIdIndexedCoinTypeenum, so we know which on-chain database to query when returning the actual coinsmaxandexcludedparams)max) we fill the remaining slots with a random number of "dust" coinsChanges to
CoinsQueryErrortypeMaxCoinsReachedvariant has been removed because in the new algorithm we never query for more coins than the specifiedmax, hence, without additional effort, we are not able to tell whether the query could be satisfied if user provided biggermaxInsufficientCoinshas been renamed toInsufficientCoinsForTheMaxand it now contains the additionalmaxfieldOff-chain database metadata
IndexationKind-CoinsToSpendRefactoring
indexation.rsmodule was split into separate files, each per indexation type + errors + some utils.Other
coinsToSpendGraphQL query is now limited to the maximum number of inputs allowed in transaction.Before requesting review
Follow-up issues
CoinsToSpendIndexKeyfromVec<u8>to typed struct, similarly toOwnedTransactionIndexKey#2498OffChainDatabase::coins_to_spend_index()function should return error if indexation is not available #2499