refactor(toolkit): add new BuilderContext trait (indexer support prep)#1605
Open
ozgb wants to merge 13 commits into
Open
refactor(toolkit): add new BuilderContext trait (indexer support prep)#1605ozgb wants to merge 13 commits into
ozgb wants to merge 13 commits into
Conversation
WIP abstraction: parameterise builders (BuildInput, BuildOutput, BuildTransient, BuildContractAction, BuildIntent, OfferInfo, StandardTrasactionInfo, SingleTxBuilder, FromContext) over a BuilderContext<D> trait instead of the concrete LedgerContext<D>. LedgerContext<D> implements BuilderContext<D>, with latest_block_context, ledger_parameters, and update_resolver still left as todo!() stubs. This is the first step toward letting toolkit drive an indexer-backed context (issue #1186) without downloading every block locally.
… guide Foundation for issue #1186 (indexer-backed toolkit). Finalizes the BuilderContext<D> trait surface (manual Pin<Box<Future>>, no async_trait): drops the dead tx_context and owned wallet_from_seed, and adds the indexer-validated query methods network_id, unshielded_utxos, zswap_state, contract_state, resolver, and a generic well_formed check. LedgerContext<D> implements every method (no stubs). utxo_output.rs (sync) and utxo_spend.rs (async) are migrated as reference templates for the remaining builders. docs/proposals/builder-context-migration.md documents the finalized trait, the indexer GraphQL API mapping, the per-file migration recipe, and the sandbox build/verify workaround. The crate does not yet compile: the remaining ~25 builder files still hold concrete Arc<LedgerContext> and are tracked in that guide.
Parameterise every tx_generator builder over a new BuilderContext<D> trait instead of a concrete LedgerContext<D>, exposing only the queries builders need (latest block context, ledger parameters, network id, unshielded UTXOs, zswap state, contract state, resolver, well-formedness, and wallet access). LedgerContext implements the trait so runtime behaviour is unchanged; an IndexerContext stub proves the trait is implementable by a non-local backend (issue #1186). Removes the now-redundant batches builder.
Replace manual `Pin<Box<dyn Future + Send + 'a>>` desugarings with the `#[async_trait]` macro on the traits that are heavily used via `dyn` (BuilderContext, BuildUtxoSpend, BuildIntent, BuildContractAction, Contract::deploy) and all of their impls. Cuts ~75 lines and removes a class of error-prone lifetime boilerplate; behavior is unchanged (ledger-helpers test suite still passes). Assisted-by: Claude:claude-4.7-opus
Resolve conflicts between BuilderContext trait abstraction and main: - Combine generic BuilderContext<D, C> signatures with main's API changes (Copy removal on WalletSeed; new dust spend/mark_spent tuple API; new --coin-selection strategy and input_utxos pinning in builders). - Migrate utxos_by_ids to BuilderContext, async. - Drop redundant batches builder (replaced by batch_single_tx). - Convert build_unshielded_intents overflow test to #[tokio::test]. Assisted-by: Claude:claude-4.7-opus Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
The BuilderContext trait refactor removed the batches builder because its inter-tx update_from_tx call is a write-side operation the trait deliberately omits. Re-add the builder pinned to the concrete LedgerContext<DefaultDB> so the existing CLI surface keeps working, update the trait-object signatures to the new <DefaultDB, C> shape, make initial_unshielded_intents async (utxos_to_cover_value is now async), and thread .clone() through call sites since WalletSeed is no longer Copy. Adds --coin-selection to BatchesArgs to match the other builders. Updates the change file and migration doc to reflect that batches was kept rather than removed. Assisted-by: Claude:claude-4.7-opus Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0901a8ba7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
…github.com> I, Oscar Bailey <79094698+ozgb@users.noreply.github.com>, hereby add my Signed-off-by to this commit: eb16d26 I, Oscar Bailey <79094698+ozgb@users.noreply.github.com>, hereby add my Signed-off-by to this commit: 6990ef2 I, Oscar Bailey <79094698+ozgb@users.noreply.github.com>, hereby add my Signed-off-by to this commit: e935344 I, Oscar Bailey <79094698+ozgb@users.noreply.github.com>, hereby add my Signed-off-by to this commit: b97f86d Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19fe8a5101
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: Oscar Bailey <79094698+ozgb@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Encapsulate the methods toolkit transaction builders use behind a new
BuilderContext<D>trait, so builders no longer depend on a concreteArc<LedgerContext<D>>that has to replay every block locally.The trait exposes only the queries builders actually need:
latest_block_context,ledger_parameters,network_id,unshielded_utxos,zswap_state,contract_state,resolver/update_resolver,well_formed, and wallet access (with_wallet_from_seed,with_wallets_from_seeds).LedgerContextimplements the trait, so runtime behaviour is unchanged. A stubIndexerContextproves the trait is implementable by a non-local backend.A follow-up PR will add the real indexer-API implementation of
BuilderContext(issue #1186), letting the toolkit drive transaction construction against a node via indexer queries instead of a fully replayed local ledger.The
batchescommand still uses the concreteLedgerContextbackend - it needs a full ledger state to pre-generate and apply transactions to create future chain states.🗹 TODO before merging
📌 Submission Checklist
git commit -s) for the DCO🧪 Testing Evidence
Existing tests via CI should not show regression
🔱 Fork Strategy
Links
IndexerContextimplementation against the indexer GraphQL API