Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add excluded_ids argument to Query.coinsToSpend #237

Merged
merged 6 commits into from
Apr 7, 2022

Conversation

AlicanC
Copy link
Contributor

@AlicanC AlicanC commented Mar 27, 2022

This PR partly implements #185. The unimplemented part was excluding coins in the txpool, but this isn't an immediately necessary feature.

@adlerjohn adlerjohn linked an issue Mar 27, 2022 that may be closed by this pull request
@AlicanC AlicanC added the enhancement New feature or request label Mar 27, 2022
@AlicanC AlicanC force-pushed the jc/coins-to-spend-exclusion branch from 5bae419 to 8b396f7 Compare April 3, 2022 21:54
@AlicanC AlicanC marked this pull request as ready for review April 4, 2022 07:27
@AlicanC AlicanC requested review from Voxelot and digorithm April 4, 2022 07:28
fuel-client/src/client.rs Outdated Show resolved Hide resolved
fuel-core/src/coin_query.rs Outdated Show resolved Hide resolved
fuel-core/src/coin_query.rs Outdated Show resolved Hide resolved
@@ -255,29 +277,26 @@ mod tests {

use super::*;

fn gen_test_db(owner: Address, asset_ids: &[AssetId]) -> Database {
static COIN_INDEX: AtomicU8 = AtomicU8::new(0);
Copy link
Member

@Voxelot Voxelot Apr 5, 2022

Choose a reason for hiding this comment

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

we might want more than a u8 as this coin index may get reused by multiple tests in the future which could easily take up more than 255 coins. Alternately, don't use a static variable here so that the index is local to each test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually want to nuke this in favor of a more proper way to generate seeded databases.

If this was the TS repo I'd write a class TestDatabase extends Database { ... } with helper methods and put the index counter in there. I'm yet to come up with a similar thing here though.

Copy link
Member

@Voxelot Voxelot Apr 7, 2022

Choose a reason for hiding this comment

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

The main issue is trying to use magic global variables. You could have a stateful db builder object that you initialize each test and call make_coin as a function on the builder. Then you could have an impl From<TestDbBuilder> for Database which creates a database and inserts all the data in one go.

Voxelot
Voxelot previously approved these changes Apr 5, 2022
@Voxelot
Copy link
Member

Voxelot commented Apr 5, 2022

Just a couple non-blocking minor nits from me, approved!

AlicanC and others added 5 commits April 6, 2022 16:10
Co-authored-by: Brandon Kite <brandonkite92@gmail.com>
Co-authored-by: Brandon Kite <brandonkite92@gmail.com>
Co-authored-by: Brandon Kite <brandonkite92@gmail.com>
@AlicanC AlicanC requested a review from Voxelot April 6, 2022 17:40
@AlicanC AlicanC merged commit 00a3936 into master Apr 7, 2022
@AlicanC AlicanC deleted the jc/coins-to-spend-exclusion branch April 7, 2022 07:53
@AlicanC AlicanC mentioned this pull request Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Exclude pending UTXOs from coin selection API
2 participants