Skip to content

Feature / Add account options to AnchorChaining#294

Open
bogdanblazhkevych wants to merge 6 commits intomainfrom
feature/use-account-options-in-fx
Open

Feature / Add account options to AnchorChaining#294
bogdanblazhkevych wants to merge 6 commits intomainfrom
feature/use-account-options-in-fx

Conversation

@bogdanblazhkevych
Copy link
Copy Markdown
Contributor

@bogdanblazhkevych bogdanblazhkevych commented May 6, 2026

Adds account and signer optional overrides to getPlans in AnchorChaining

Adds options to KeetaFXAnchorProviderBase which can pass an account to block builder options in createExchange.

Copy link
Copy Markdown

@chpzcch chpzcch left a comment

Choose a reason for hiding this comment

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

I've reviewed the changes. The implementation of account overrides for FX exchanges and AnchorChaining looks solid and is well-supported by new tests.

I found one suspicious removal of an override in the asset movement planning stage and a minor syntax issue.

Please see the inline comments for details.

Copy link
Copy Markdown

@chpzcch chpzcch left a comment

Choose a reason for hiding this comment

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

Added inline comments for specific concerns.

Comment thread src/lib/chaining.ts Outdated
@@ -1128,7 +1142,7 @@ export class AnchorChainingPlan extends AnchorChainingPath {
type: 'assetMovement',
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why was options?.overrides removed from this getAccountForAction call? In the previous version it was present, and it is still used in the FX step resolution above. If an override is provided for asset movement, it should likely be honored here as well to ensure consistent plan validation.

Comment thread src/services/fx/client.ts
this.providerID = providerID;
this.conversion = conversion;
this.parent = parent;
this.options = options
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing semicolon.

@chpzcch
Copy link
Copy Markdown

chpzcch commented May 7, 2026

Code Review Summary

Issues reviewed: PR 294 — Feature / use account option in FX Client and AnchorChaining. No linked issues were provided in the PR description.

Threads addressed: 0 threads reviewed (no existing threads).

New issues found:

Must Fix

  • In src/lib/chaining.ts, options?.overrides was removed from a getAccountForAction call during asset movement planning. This appears to be a regression or mistake, as overrides should be honored consistently.

Minor / Optional

  • Missing semicolon in src/services/fx/client.ts at line 245.

⚠️ Analytics Impact

Event registry (/shared/analytics.ts/docs/ANALYTICS_EVENTS.md):
Checked for registry files, but they do not exist in this repository. No PostHog event calls (posthog.capture) were found in the diff.

PostHog events and funnels:
None detected.

Admin analytics:
None detected.

What's working well

  • The addition of AccountOptions to the FX client and AnchorChaining correctly enables support for storage accounts and other non-default accounts.
  • The new tests in chaining.test.ts and client.test.ts are comprehensive, covering state transitions, event emissions, and balance verification for storage accounts.

Overall assessment:
The PR successfully implements the requested flexibility for account handling in FX and chaining operations. The core logic is sound and well-tested. Once the missing override in chaining.ts is addressed, it should be safe to merge.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 8, 2026

@bogdanblazhkevych bogdanblazhkevych changed the title Feature / use account option in FX Client and AnchorChaining Feature / Add account options to AnchorChaining May 8, 2026
@rkeene rkeene requested a review from Copilot May 8, 2026 16:13
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the FX and chaining flows to support optional account/signer overrides, enabling exchanges (and subsequent chaining execution) to be performed using non-default accounts such as storage accounts.

Changes:

  • Thread account options through KeetaFXAnchorClient provider instances so createExchange() can build blocks using a specified account.
  • Add ComputePlanOptions to AnchorChaining.getPlans() and propagate overrides into FX quote/estimate lookup and into execution-time sends.
  • Expand chaining tests (and FX client tests) to validate execution using storage accounts and update transfer-status expectations.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/services/fx/client.ts Adds optional account options to provider instances and passes them into block builder operations in createExchange.
src/services/fx/client.test.ts Adds a storage-account test ensuring FX exchanges can complete using overridden account funds.
src/lib/chaining.ts Exposes ComputePlanOptions, adds account/signer override resolution helpers, and uses overrides during planning/execution.
src/lib/chaining.test.ts Adds chaining execution coverage for storage-account overrides and updates mocked transfer statuses.

Comment thread src/lib/chaining.ts
Comment thread src/lib/chaining.ts
}

await this.parent['client'].send(sendToAddress, value, token, external);
const { account } = await this.getAccountsForAction({ type: 'assetMovement', providerMethod: 'initiateTransfer' }, this.#_plan?.options?.overrides);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants