Add support of messages into `coinsToSpend` #591

xgreenx · 2022-09-04T02:13:15Z

Created a new entity - Resource. It is an aggregator for the coins and messages. For more information, check the discussion at the #586

The API looks like:

struct SpendQueryElementInput {
    /// Asset ID to spend
    asset_id: AssetId,
    /// Target amount for the query
    amount: U64,
    /// The maximum number of resources for selection.
    max: Option<U64>,
}

pub struct ExcludeInput {
    /// Utxos to exclude from the selection.
    utxos: Vec<UtxoId>,
    /// Messages to exclude from the selection.
    messages: Vec<MessageId>,
}

pub enum Resource {
    Coin(Coin),
    Message(Message),
}

fn resourcesToSpend(
    // The `Address` of the resources owner.
    owner: Address, 
    // The list of requested assets` resources with asset ids, `target` amount the user wants
    // to reach, and the `max` number of resources in the selection. Several entries with the
    // same asset id are not allowed.
    query_per_asset: Vec<SpendQueryElementInput>, 
    // The excluded resources from the selection.
    exclude: Option<ExcludeInput>,
)
// The list of spendable resources per asset from the query. The length of the result is
// the same as the length of `query_per_asset`. The ordering of assets and `query_per_asset`
// is the same.
-> Result<Vec<Vec<Resource>>>

Added some abstraction to remove logic duplicating. I want to reuse it in the other schemas like balance.
Added schema::Scalar for MessageId.
Added message_id for the schema::Message.

Closes #586

…and messages. We can't use `Input` or `Output` naming because we already do that with transactions. So I introduced a new one. We can think about the naming in the PR=) Renamed `coinsToSpent` into `banknotesToSpent`. Move it to a separate file. Changed the signature. Not it returns `Vec<Vec<Banknote>>` instead of `Vec<Coin>`. The result is a list of `Banknote` per asset(we can have many different assets, so it is why it has an additional vector). Replaced exclude vector with two vectors in the separate struct. One vector allows filtering by utxos, another by messages. Reduced the number of operations in the `banknotesToSpent`. Added some abstraction to remove logic duplicating. I want to reuse it in the other schemas like `balance`. Added `schema::Scalar` for `MessageId`. I need to add the support of `Message` into tests and the code. I will do it in the next commit=)

ControlCplusControlV · 2022-09-04T03:24:52Z

I see a lot of API changes in here, are you sure of how this affects downstream consumers of fuel-core yet?

xgreenx · 2022-09-04T09:57:25Z

Do you ask about coinsToSpend method signature or changes also in the database?

coinsToSpend is an inevitable change because we want to work with different objects(coins, messages). Because it breaks API, we can make more changes to make the API clear. For example, return banknotes per asset, and pass max_input per asset.

The changes in the database it forcing people to change how they work with it to have more optimal code.

ControlCplusControlV · 2022-09-04T14:54:45Z

Do you ask about coinsToSpend method signature or changes also in the database?

coinsToSpend is an inevitable change because we want to work with different objects(coins, messages). Because it breaks API, we can make more changes to make the API clear. For example, return banknotes per asset, and pass max_input per asset.

Changing to coinsToSpend I agree is inevitable for the best, I am more referring to the changes on both endpoint name and in the GQL schema. Any breaking issues there should be identified and a fix prepped there before merging this though to avoid issue

xgreenx · 2022-09-05T14:12:28Z

Before I will update the tests let's discuss the final query signature in the issue=)

# Conflicts: # fuel-client/src/client.rs # fuel-core/src/coin_query.rs # fuel-core/src/database/coin.rs # fuel-core/src/schema/coin.rs # fuel-tests/tests/coin.rs

Uncommented and fixed old tests.

…m to work with coins, messages, and coins and messages test cases. Added new field to `schema::Message` - `message_id`. It is calculated based on the `Message`. Added check of errors in the client's tests.

fuel-client/src/client/schema/primitives.rs

Voxelot · 2022-09-13T20:50:27Z

fuel-core/src/database/utils.rs

@@ -0,0 +1,218 @@
+use crate::{


I'd prefer to avoid a generic utils module, as it makes organization less clear and will just keep growing with potentially unrelated things.

Since this is mostly about coins and messages, maybe rename this utils module into resources within the database instead?

Voxelot · 2022-09-13T20:54:46Z

fuel-core/src/coin_query.rs

+    let mut collected_amount = 0u64;
+    let mut resources = vec![];
+
+    for coin in inputs {


Suggested change

for coin in inputs {

for resource in inputs {

Voxelot · 2022-09-13T20:56:45Z

fuel-core/src/database/utils.rs

+
+/// At least required `target` of the query per asset's `id` with `max` resources.
+#[derive(Clone)]
+pub struct Asset {


This is a selection criteria, not an asset 🤔

Maybe something like AssetSpendTarget?

Voxelot · 2022-09-13T21:00:51Z

fuel-core/src/database/utils.rs

+/// The primary type of spent or not spent resources(coins, messages, etc). The not spent resource
+/// can be used as a source of information for the creation of the transaction's input.
+#[derive(Debug)]
+pub enum Resource<C, M> {


Since the schema has its own analog to resource, why include these generics here?

To avoid unnecessary Copy and Clone. The query to DB returns Resource<Cow<Coin>, Cow<Message>>. But the graphql query returns Resource<Coin, Message>.

Since the schema has its own analog to resource

It looks like the schema doesn't rely on this enum, since it has it's own: https://github.com/FuelLabs/fuel-core/blob/feature/coins-to-spent-messages/fuel-core/src/schema/resource.rs#L52

Couldn't we just simplify this type by making it always a Cow?

No, because Cow requires lifetime from DB, and in the resources_to_spend we can't return Vec<Vec<Resource<Cow, Cow>>>=(

Do we really need Cow? Or is it always owned once deserialized from the DB anyways? Even though the storage trait uses Cow, I don't think it would ever be borrowed unless we had some kind of zero-copy deserialization going on.

Yes, if we use RocksDB, in this case, it is useless=) I tried to be consistent with our API. Database returns Cow, and in some cases (maybe in the future), it can be useful.

If you think it adds unjustified complexity, I can remove it=)

eh it's not a big deal. I just prefer to find ways to hide generics when possible since it can get out of hand sometimes (like in substrate codebases lol)

fuel-core/src/database/coin.rs

fuel-core/src/coin_query.rs

fuel-core/src/test_utils.rs

Voxelot · 2022-09-13T22:00:04Z

fuel-core/src/state/rocks_db.rs

@@ -221,6 +221,8 @@ impl KeyValueStore for RocksDb {
            .iterator_cf_opt(&self.cf(column), opts, iter_mode)
            .map(|item| {
                item.map(|(key, value)| {
+                    // TODO: Avoid copy of the slices into vector.


We could also adjust the API to make use of boxed slices instead of vec 🤔

We can, the question do we want to add complexity to database API=) But it will force us to work with raw slice where it is possible=)

Voxelot · 2022-09-13T22:03:44Z

fuel-tests/tests/resource.rs

+            .unwrap();
+        assert_eq!(resources_per_asset.len(), 2);
+        assert_eq!(resources_per_asset[0].len(), 3);
+        assert_eq!(resources_per_asset[1].len(), 3);


should we also check that the returned resources add up to >= target in these tests?

Voxelot · 2022-09-13T22:24:54Z

Done reviewing for now, will wait for the requested changes before approving.

leviathanbeak · 2022-09-14T17:21:39Z

fuel-core/src/schema/balance.rs

@@ -76,8 +76,9 @@ impl BalanceQuery {
    ) -> async_graphql::Result<Balance> {
        let db = ctx.data_unchecked::<Database>();

+        // TODO: Reuse [`AssetQuery`](crate::database::utils::AssetQuery) with messages


nit: would be nice to see Issues created for these TODOs (if not already)

leviathanbeak

LGTM, there are handful of TODOs, would be nice that Issues are created for and possibly referenced

ControlCplusControlV

LGTM, will move new coinsToSpend tests over to my rstest PR once merged. We may also want to look at organizing fuel-tests in a future issue as it is mostly flat right now and could benefit from tests being organized into folders

Also +1 on moving TODOs to issues (with TODO comment referencing the issue #)

Renamed `CoinQueryError` into `ResourceQueryError`. Added `asset_id` and `collected_amount` into `ResourceQueryError`.

# Conflicts: # fuel-core/src/coin_query.rs # fuel-core/src/database.rs # fuel-core/src/database/coin.rs # fuel-core/src/database/message.rs # fuel-core/src/state.rs # fuel-core/src/test_utils.rs

xgreenx self-assigned this Sep 4, 2022

xgreenx requested a review from Voxelot September 4, 2022 10:01

xgreenx changed the title ~~Add support of messages into coinsToSpent~~ Add support of messages into coinsToSpend Sep 6, 2022

xgreenx added 6 commits September 12, 2022 12:11

Merge branch 'master' into feature/coins-to-spent-messages

4c0c0ed

# Conflicts: # fuel-client/src/client.rs # fuel-core/src/coin_query.rs # fuel-core/src/database/coin.rs # fuel-core/src/schema/coin.rs # fuel-tests/tests/coin.rs

Merged with master

e7cdfd8

Applied changes from the issue

6872e8f

Added messages into the unspent_resources result.

224cc45

Uncommented and fixed old tests.

Make clippy happy)

b0c52d3

Make clippy happy)

6b9e0ec

xgreenx mentioned this pull request Sep 12, 2022

Include messages in balance API #614

Closed

xgreenx added breaking A breaking api change graphql-api Affects API of the GraphQL labels Sep 12, 2022

xgreenx added 2 commits September 12, 2022 23:36

Added unit tests for Message. I used old test-cases and adapted the…

012b528

…m to work with coins, messages, and coins and messages test cases. Added new field to `schema::Message` - `message_id`. It is calculated based on the `Message`. Added check of errors in the client's tests.

Updated teh comment

3dbf5a0

xgreenx marked this pull request as ready for review September 12, 2022 23:16

xgreenx and others added 3 commits September 13, 2022 02:41

Merge branch 'master' into feature/coins-to-spent-messages

4e740d8

Increase timeout to 45, if it helps, I will create a separate PR

386e834

Merge branch 'master' into feature/coins-to-spent-messages

c160648