Refactor SdkBuilder for clarity#875
Open
dangeross wants to merge 2 commits into
Open
Conversation
JssDWt
reviewed
May 12, 2026
Collaborator
JssDWt
left a comment
JssDWt
left a comment
There was a problem hiding this comment.
LGTM apart from these comment suggestions. We should add some guidance about comments to CLAUDE.md.
| Some((pool.inner.clone(), identity, pool.foreign_key_mode)) | ||
| /// Builds a fully-populated [`SparkWalletConfig`](spark_wallet::SparkWalletConfig) | ||
| /// from the SDK [`Config`], including the user-agent string and per-SDK | ||
| /// overrides (optimisation, max concurrent claims). |
Collaborator
There was a problem hiding this comment.
Suggested change
| /// overrides (optimisation, max concurrent claims). | |
| /// overrides. |
Comment on lines
+653
to
+656
| /// | ||
| /// Lines count is over the default clippy threshold because each | ||
| /// extracted helper is invoked with a struct-literal argument list; the | ||
| /// body itself is a top-to-bottom orchestration of named assembly steps. |
Collaborator
There was a problem hiding this comment.
Suggested change
| /// | |
| /// Lines count is over the default clippy threshold because each | |
| /// extracted helper is invoked with a struct-literal argument list; the | |
| /// body itself is a top-to-bottom orchestration of named assembly steps. |
Comment on lines
+131
to
+133
| /// Parameters for [`build_spark_wallet`]. Grouped into a struct because the | ||
| /// `WalletBuilder::with_*` chain has too many distinct knobs for a flat | ||
| /// parameter list. |
Collaborator
There was a problem hiding this comment.
Suggested change
| /// Parameters for [`build_spark_wallet`]. Grouped into a struct because the | |
| /// `WalletBuilder::with_*` chain has too many distinct knobs for a flat | |
| /// parameter list. | |
| /// Parameters for [`build_spark_wallet`]. |
|
|
||
| // Create the base signer based on the signer source | ||
| let signer: Arc<dyn crate::signer::BreezSigner> = match self.signer_source { | ||
| /// Builds the four [`Signers`] used by the SDK from a single base signer. |
Collaborator
There was a problem hiding this comment.
Suggested change
| /// Builds the four [`Signers`] used by the SDK from a single base signer. | |
| /// Builds the [`Signers`] used by the SDK from a single base signer. |
Comment on lines
+687
to
+690
| #[cfg(not(all(target_family = "wasm", target_os = "unknown")))] | ||
| let storage_components = build_storage_components(storage_components_params).await?; | ||
| #[cfg(all(target_family = "wasm", target_os = "unknown"))] | ||
| let storage_components = build_storage_components(storage_components_params)?; |
Collaborator
There was a problem hiding this comment.
Maybe the wasm build_storage_components function can also be async, with a clippy suppression, to avoid the extra cfg switch here.
Comment on lines
+822
to
+824
| /// Reads the integrator-supplied SQL connection pool (if any) and pairs it | ||
| /// with the tenant identity derived from the spark signer, so storage, tree | ||
| /// store, token store, and session manager all scope to the same row space. |
Collaborator
There was a problem hiding this comment.
I object to this comment. It should say something like 'Initialize the storage backends'. "scope to the same row space" almost doesn't mean anything to a human.
Comment on lines
+900
to
+905
| /// Builds every persistence-layer component used by the SDK from one | ||
| /// backend selection. Resolution order for each store: user-injected → | ||
| /// postgres pool → mysql pool. The session manager is wrapped in | ||
| /// [`EncryptingSessionManager`](crate::session_manager::EncryptingSessionManager) | ||
| /// and [`CachingSessionManager`](crate::session_manager::CachingSessionManager) | ||
| /// via [`wrap_session_manager`]. |
Collaborator
There was a problem hiding this comment.
Suggested change
| /// Builds every persistence-layer component used by the SDK from one | |
| /// backend selection. Resolution order for each store: user-injected → | |
| /// postgres pool → mysql pool. The session manager is wrapped in | |
| /// [`EncryptingSessionManager`](crate::session_manager::EncryptingSessionManager) | |
| /// and [`CachingSessionManager`](crate::session_manager::CachingSessionManager) | |
| /// via [`wrap_session_manager`]. | |
| /// Builds every persistence-layer component used by the SDK from one | |
| /// backend selection. |
dangeross
force-pushed
the
mysql-foreign-key
branch
from
2c2b708 to
24d419c
Compare
dangeross
force-pushed
the
savage-sdk-builder-refactor
branch
from
3ad3b40 to
b92f5eb
Compare
dangeross
force-pushed
the
mysql-foreign-key
branch
from
24d419c to
0cf2cc6
Compare
dangeross
force-pushed
the
savage-sdk-builder-refactor
branch
from
b92f5eb to
ac71889
Compare
dangeross
force-pushed
the
mysql-foreign-key
branch
2 times, most recently
from
e710a4f to
94b2f22
Compare
dangeross
force-pushed
the
savage-sdk-builder-refactor
branch
from
ac71889 to
bc8e6bb
Compare
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
3 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.
Refactors
SdkBuilder::build()(core + WASM) to read as a top-to-bottom sequence of named assembly steps.Core
build()is broken into focused helpers —build_signers,default_chain_service,validate_storage_sources,resolve_storage_backends,build_storage_components,finalize_spark_wallet_config,build_spark_wallet,resolve_lnurl_server_client,maybe_wrap_storage_with_real_time_sync,build_token_converter,build_stable_balance. The four scattered storage ladders (storage / tree store / token store / session manager) collapse into one place; the postgres/mysql backend tuples become typedPostgresBackend/MysqlBackendstructs.WASM
build()shrinks to ~25 viaderive_identity_pub_key(removes 3x KeySet boilerplate) andapply_postgres_pool/apply_mysql_pool(folds the near-identical pool branches).