fuel-core icon indicating copy to clipboard operation
fuel-core copied to clipboard

Add support of messages into `coinsToSpent`

Open xgreenx opened this issue 2 years ago • 3 comments

Created a new entity - banknote. It is an aggregator for the coins 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=)

Closes https://github.com/FuelLabs/fuel-core/issues/586

xgreenx avatar Sep 04 '22 02:09 xgreenx

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

ControlCplusControlV avatar Sep 04 '22 03:09 ControlCplusControlV

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.

xgreenx avatar Sep 04 '22 09:09 xgreenx

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

ControlCplusControlV avatar Sep 04 '22 14:09 ControlCplusControlV

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

xgreenx avatar Sep 05 '22 14:09 xgreenx

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

Voxelot avatar Sep 13 '22 22:09 Voxelot