Feature/active options positions by zahorodnyi · Pull Request #57 · Blockstream/simplicity-dex

zahorodnyi · 2026-02-11T16:57:15Z

Implement a basic feature to display users’ active options positions.

Resolve: #41 (comment)

The feature currently displays the following information about active options:

Table index
User's role
User's option tokens amount
User's grantor tokens amount
Contract expiry time
Contract ID

zahorodnyi · 2026-02-11T17:02:03Z

@greptile review the PR according to the CONTRIBUTION.md

greptile-apps · 2026-02-11T17:33:49Z

Greptile Summary

This PR implements a new "Active Options Positions" table displaying each options contract the user is involved in, with their role (maker/taker), token amounts, expiry, and contract address. The feature works by parsing stored contracts, aggregating token balances, querying locked offer collateral, and filtering by contract validity and activity state.

Key concern: The maker role determination relies on outpoint txid matching the contract funding txid. This is fragile — any UTXO consolidation or token movement after becoming the maker will cause the txid to change, silently misidentifying the user as a taker. This is a user-facing correctness issue affecting the core feature.

Implementation quality: The code is well-structured with clear helper functions for parsing, aggregating, and filtering contract states. The commit history is clean (2 logical, self-contained commits).

Confidence Score: 2/5

This PR has a correctness issue that silently produces wrong user-facing results: maker detection breaks after UTXO consolidation.
The PR introduces a core feature (active options positions table) with a fragile maker/taker role determination that relies on UTXO outpoint txid matching. Since users' wallets naturally consolidate UTXOs and move tokens, this issue will likely manifest in real usage, causing makers to be displayed incorrectly as takers. This is not just a potential edge case but a common wallet operation that breaks the feature. Other code quality is good, but this correctness issue in the primary feature being introduced significantly impacts the confidence score.
crates/cli-client/src/cli/positions.rs — specifically the maker detection logic around line 499-507.

Important Files Changed

Filename	Overview
crates/cli-client/src/cli/positions.rs	Large implementation of active options position tracking. Primary concern: maker role detection is fragile, relying on UTXO outpoint txid matching the contract funding txid (line 505). Any UTXO consolidation or movement after funding will cause silent misidentification of the maker as a taker. This affects user-facing correctness in the core feature. Otherwise the code is well-structured with clear separation of concerns and proper async patterns.
crates/cli-client/src/cli/tables.rs	Adds `TableData` impl for `ActiveOptionsDisplay` and the `display_active_options_table` function. Change is minimal, straightforward, and follows existing patterns for display implementations.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[run_positions] --> B[fetch BTC price]
    A --> C[get_option_tokens_from_wallet]
    A --> D[get_grantor_tokens_from_wallet]
    C & D --> E[build_active_options_displays]
    E --> F[fetch_active_contract_states]
    F --> G[list_contracts_by_source OPTION_SOURCE]
    F --> H[parse_option_contracts]
    H --> I[token_to_contract map]
    F --> J[aggregate_token_balances\noption & grantor]
    F --> K[query_contract_tokens_in_offers\nlist_contracts_by_source OPTION_OFFER_SOURCE]
    K --> L[map_offers_to_contracts]
    F --> M{candidate_ids\noption + grantor + offer contracts}
    J & L --> M
    M --> N[query_collateral_for_candidates]
    N --> O[build_and_filter_contract_states\nis_valid + is_active]
    O --> P[sort by expiry_time]
    P --> Q[display_active_options_table]
    A --> R[list_contracts OPTION_SOURCE again\nfor Contract History]
    A --> S[list_contracts OPTION_OFFER_SOURCE again\nfor Contract History]

_{Last reviewed commit: b9586f1}

greptile-apps

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-11T17:33:52Z

crates/cli-client/src/cli/positions.rs

+/// Query all option tokens locked in option-offer contracts for multiple asset IDs
+async fn query_all_option_tokens_in_offers(
+    wallet: &crate::wallet::Wallet,


Incorrect offer token accounting

query_all_option_tokens_in_offers is described/used as summing “option tokens locked in option-offer contracts”, but option-offer contracts lock collateral/premium (and later settlement), not option tokens. In this function you filter and sum using OptionOfferArguments::get_collateral_asset_id() and UtxoFilter::asset_id(collateral_id) (positions.rs:237-248), so option_tokens_in_offers is actually collateral-in-offers keyed by the collateral asset id. Downstream, build_maker_positions treats that map as “unsold option tokens” (option_tokens_in_offers.get(&option_asset_id) at positions.rs:371-372), so Maker “sold of total” will be wrong (typically under/over-counting depending on assets).

Also appears at positions.rs:213-274 and positions.rs:367-383.

Prompt To Fix With AI

This is a comment left during a code review. Path: crates/cli-client/src/cli/positions.rs Line: 213:215 Comment: **Incorrect offer token accounting** `query_all_option_tokens_in_offers` is described/used as summing “option tokens locked in option-offer contracts”, but option-offer contracts lock *collateral/premium* (and later settlement), not option tokens. In this function you filter and sum using `OptionOfferArguments::get_collateral_asset_id()` and `UtxoFilter::asset_id(collateral_id)` (positions.rs:237-248), so `option_tokens_in_offers` is actually *collateral-in-offers* keyed by the collateral asset id. Downstream, `build_maker_positions` treats that map as “unsold option tokens” (`option_tokens_in_offers.get(&option_asset_id)` at positions.rs:371-372), so Maker “sold of total” will be wrong (typically under/over-counting depending on assets). Also appears at positions.rs:213-274 and positions.rs:367-383. How can I resolve this? If you propose a fix, please make it concise.

zahorodnyi · 2026-02-18T17:02:41Z

@greptile review the PR according to the CONTRIBUTION.md

greptile-apps

_{2 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

crates/cli-client/src/cli/positions.rs

zahorodnyi · 2026-02-18T17:15:47Z

@greptile review the PR according to the CONTRIBUTION.md

greptile-apps

_{2 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-18T17:25:09Z

crates/cli-client/src/cli/positions.rs

+    let (option_balances, grantor_balances, tokens_in_offers) = tokio::join!(
+        async { aggregate_token_balances(option_tokens) },
+        async { aggregate_token_balances(grantor_tokens) },
+        query_option_tokens_in_offers(wallet, &option_asset_ids, network)
+    );


Unnecessary async wrapping of sync functions

aggregate_token_balances is a synchronous, CPU-bound function. Wrapping it in async { ... } and passing it to tokio::join! provides no concurrency benefit — the async block completes immediately on first poll. Only query_option_tokens_in_offers is genuinely async here.

Consider either calling the sync functions directly before the async work, or adding a comment explaining the intent:

Suggested change

let (option_balances, grantor_balances, tokens_in_offers) = tokio::join!(

async { aggregate_token_balances(option_tokens) },

async { aggregate_token_balances(grantor_tokens) },

query_option_tokens_in_offers(wallet, &option_asset_ids, network)

);

let option_balances = aggregate_token_balances(option_tokens);

let grantor_balances = aggregate_token_balances(grantor_tokens);

let tokens_in_offers = query_option_tokens_in_offers(wallet, &option_asset_ids, network).await;

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Prompt To Fix With AI

This is a comment left during a code review. Path: crates/cli-client/src/cli/positions.rs Line: 453-457 Comment: **Unnecessary async wrapping of sync functions** `aggregate_token_balances` is a synchronous, CPU-bound function. Wrapping it in `async { ... }` and passing it to `tokio::join!` provides no concurrency benefit — the `async` block completes immediately on first poll. Only `query_option_tokens_in_offers` is genuinely async here. Consider either calling the sync functions directly before the async work, or adding a comment explaining the intent: ```suggestion let option_balances = aggregate_token_balances(option_tokens); let grantor_balances = aggregate_token_balances(grantor_tokens); let tokens_in_offers = query_option_tokens_in_offers(wallet, &option_asset_ids, network).await; ``` <sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub> How can I resolve this? If you propose a fix, please make it concise.

zahorodnyi · 2026-02-18T17:41:31Z

@greptile review the PR according to the CONTRIBUTION.md

greptile-apps

_{2 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-18T17:47:21Z

crates/cli-client/src/cli/positions.rs

+        let contract_id = tpg_str.clone();
+        option_asset_to_contract.insert(args.option_token(), contract_id.clone());


Missing grantor token to contract mapping

Only args.option_token() is inserted into option_asset_to_contract, but grantor tokens are never mapped. This means the option_asset_ids set passed into query_option_tokens_in_offers will only contain option token asset IDs, not grantor token asset IDs. If a grantor token is ever used as collateral in an option-offer, it won't be discovered, and locked_in_offers will undercount.

Additionally, the map_offers_to_contracts function relies on this same map to resolve asset IDs back to contract IDs — any offer collateral that happens to be a grantor token would be silently dropped.

Consider also mapping the grantor token via get_grantor_token_ids() alongside the option token.

Prompt To Fix With AI

This is a comment left during a code review. Path: crates/cli-client/src/cli/positions.rs Line: 344:345 Comment: **Missing grantor token to contract mapping** Only `args.option_token()` is inserted into `option_asset_to_contract`, but grantor tokens are never mapped. This means the `option_asset_ids` set passed into `query_option_tokens_in_offers` will only contain option token asset IDs, not grantor token asset IDs. If a grantor token is ever used as collateral in an option-offer, it won't be discovered, and `locked_in_offers` will undercount. Additionally, the `map_offers_to_contracts` function relies on this same map to resolve asset IDs back to contract IDs — any offer collateral that happens to be a grantor token would be silently dropped. Consider also mapping the grantor token via `get_grantor_token_ids()` alongside the option token. How can I resolve this? If you propose a fix, please make it concise.

crates/cli-client/src/cli/positions.rs

zahorodnyi · 2026-02-19T12:18:03Z

@greptile review the PR according to the CONTRIBUTION.md

greptile-apps

_{2 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

crates/cli-client/src/cli/positions.rs

KyrylR · 2026-02-23T12:12:36Z

crates/cli-client/src/cli/positions.rs

+    fn is_active(&self) -> bool {
+        let collateral_per_contract = self.args.collateral_per_contract();
+        if collateral_per_contract == 0 {
+            return false;
+        }
+        let total_issued = self.total_collateral / collateral_per_contract;
+        let user_share = self
+            .user_options
+            .saturating_add(self.user_grantors)
+            .saturating_add(self.locked_in_offers);
+        let held_by_others = total_issued.saturating_mul(2).saturating_sub(user_share);
+        held_by_others > 0
+    }


Let's checked_* functions instead of saturating_*

KyrylR · 2026-02-23T12:26:07Z

crates/cli-client/src/cli/positions.rs

+impl ContractState {
+    /// A contract is valid if it has not expired and has collateral locked.
+    fn is_valid(&self, now: i64) -> bool {
+        let expiry = i64::from(self.args.expiry_time());


Suggested change

let expiry = i64::from(self.args.expiry_time());

let expiry = <use method from below to get expiry time>;

zahorodnyi · 2026-03-06T18:56:06Z

@greptile review the PR according to the CONTRIBUTION.md

greptile-apps · 2026-03-06T19:01:23Z

crates/cli-client/src/cli/positions.rs

+    let maker_contract_ids: HashSet<String> = option_tokens
+        .iter()
+        .chain(grantor_tokens.iter())
+        .filter_map(|entry| {
+            let parsed = parsed_contracts.get(&entry.taproot_pubkey_gen_str)?;
+            let funding_txid = parsed.funding_txid.as_deref()?;
+            (entry.entry.outpoint().txid.to_string() == funding_txid).then(|| entry.taproot_pubkey_gen_str.clone())
+        })
+        .collect();


Maker detection breaks after token UTXO consolidation

The maker identification compares entry.entry.outpoint().txid to parsed.funding_txid. This works only while the maker holds the exact UTXO created during funding. Any on-chain operation that spends that UTXO — such as wallet dust consolidation, token re-transmission, or UTXO merging — produces a new txid, causing the comparison to fail silently. The maker will then be misidentified as a taker in the positions table.

Consider deriving the maker role from a more stable property. Options include:

Recording a maker flag in contract metadata at funding time and reading it back

Deriving the role from whether the user's pubkey matches the contract's maker pubkey field in OptionsArguments

Prompt To Fix With AI

This is a comment left during a code review. Path: crates/cli-client/src/cli/positions.rs Line: 499-507 Comment: **Maker detection breaks after token UTXO consolidation** The maker identification compares `entry.entry.outpoint().txid` to `parsed.funding_txid`. This works only while the maker holds the exact UTXO created during funding. Any on-chain operation that spends that UTXO — such as wallet dust consolidation, token re-transmission, or UTXO merging — produces a new txid, causing the comparison to fail silently. The maker will then be misidentified as a `taker` in the positions table. Consider deriving the maker role from a more stable property. Options include: - Recording a maker flag in contract metadata at funding time and reading it back - Deriving the role from whether the user's pubkey matches the contract's maker pubkey field in `OptionsArguments` How can I resolve this? If you propose a fix, please make it concise.

zahorodnyi requested a review from KyrylR as a code owner February 11, 2026 16:57

zahorodnyi self-assigned this Feb 11, 2026

zahorodnyi added the greptile Mark this PR to be reviewed by Greptile label Feb 11, 2026

greptile-apps bot reviewed Feb 11, 2026

View reviewed changes

zahorodnyi marked this pull request as draft February 12, 2026 15:33

zahorodnyi force-pushed the feature/active-options-positions branch 2 times, most recently from e8d39c9 to 3370cb2 Compare February 18, 2026 16:49

greptile-apps bot reviewed Feb 18, 2026

View reviewed changes

crates/cli-client/src/cli/positions.rs Outdated Show resolved Hide resolved

crates/cli-client/src/cli/positions.rs Show resolved Hide resolved

zahorodnyi force-pushed the feature/active-options-positions branch from 3370cb2 to 9e7fdd6 Compare February 18, 2026 17:14

greptile-apps bot reviewed Feb 18, 2026

View reviewed changes

zahorodnyi force-pushed the feature/active-options-positions branch from 9e7fdd6 to 56cdd41 Compare February 18, 2026 17:40

greptile-apps bot reviewed Feb 18, 2026

View reviewed changes

zahorodnyi force-pushed the feature/active-options-positions branch from 56cdd41 to 7a54d11 Compare February 19, 2026 12:12

greptile-apps bot reviewed Feb 19, 2026

View reviewed changes

crates/cli-client/src/cli/positions.rs Outdated Show resolved Hide resolved

zahorodnyi marked this pull request as ready for review February 19, 2026 12:32

KyrylR reviewed Feb 23, 2026

View reviewed changes

Implement active options positions feature

c529813

zahorodnyi force-pushed the feature/active-options-positions branch from 7a54d11 to c529813 Compare February 24, 2026 14:54

zahorodnyi marked this pull request as draft March 6, 2026 10:55

Add role to user active options positions and fix active-state logic

b9586f1

greptile-apps bot reviewed Mar 6, 2026

View reviewed changes

		let contract_id = tpg_str.clone();
		option_asset_to_contract.insert(args.option_token(), contract_id.clone());

	let expiry = i64::from(self.args.expiry_time());
	let expiry = <use method from below to get expiry time>;

Conversation

zahorodnyi commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zahorodnyi commented Feb 11, 2026

Uh oh!

greptile-apps bot commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 2/5

Important Files Changed

Flowchart

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

zahorodnyi commented Feb 18, 2026

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zahorodnyi commented Feb 18, 2026

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

zahorodnyi commented Feb 18, 2026

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps bot Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zahorodnyi commented Feb 19, 2026

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KyrylR Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

KyrylR Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

zahorodnyi commented Mar 6, 2026

Uh oh!

greptile-apps bot Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zahorodnyi commented Feb 11, 2026 •

edited

Loading

greptile-apps bot commented Feb 11, 2026 •

edited

Loading