fix(client): resolve 8 architectural issues in client wallet provider layer#225
fix(client): resolve 8 architectural issues in client wallet provider layer#225solidsnakedev wants to merge 1 commit intomainfrom
Conversation
… layer - Issue 1: fix spread order so provider.submitTx wins over wallet.submitTx on both promise and Effect surfaces in createSigningClient - Issue 2: remove duplicate ProviderError from Client.ts (shadowed canonical one); re-export Provider.ProviderError from index.ts instead - Issue 3: add missing Conway cert types (RegDrep, UnregDrep, UpdateDrep, AuthCommitteeHot, ResignCommitteeCold, PoolRegistration, PoolRetirement) to computeRequiredKeyHashesSync - Issue 4: narrow wallet-only createClient overload from AnyWallet to SigningWallet | WalletFactory - Issue 5: dispatch on address type in signMessage — reward addresses use stake key per CIP-0008; payment addresses use payment key - Issue 6: narrow ApiWalletEffect.signTx context to exclude referenceUtxos since CIP-30 wallets cannot use reference inputs - Issue 7: replace mutable address cache vars in cip30Wallet with lazy Promise-based cache to eliminate fiber race condition - Issue 8: replace spread-based effectInterface in createSigningClient with explicit field assignments typed as SigningClientEffect; use .bind() to forward provider/wallet methods without implicit-any parameters
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up fix to the evolution SDK’s client wallet/provider composition layer, addressing architectural issues found after #224 to make routing and signing behavior more correct and less fragile.
Changes:
- Makes
createSigningClientcomposition deterministic (provider overrides wallet forsubmitTx; Effect surface built explicitly to prevent silent overrides). - Expands required-signer detection for additional Conway-era certificate types, and updates message signing to use the stake key for reward addresses (CIP-0008).
- Tightens types around client creation overloads and CIP-30 wallet signing context (removing unsupported
referenceUtxos).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/evolution/src/sdk/wallet/WalletNew.ts | Narrows CIP-30 ApiWalletEffect.signTx context to exclude referenceUtxos. |
| packages/evolution/src/sdk/client/Wallets.ts | Extends required-signer detection for more certs, fixes signMessage key selection, and refactors CIP-30 address caching. |
| packages/evolution/src/sdk/client/ClientImpl.ts | Fixes spread-order routing and replaces Effect spread composition with explicit SigningClientEffect forwarding. |
| packages/evolution/src/sdk/client/Client.ts | Removes duplicate ProviderError definition and tightens Effect typings formatting. |
| packages/evolution/src/index.ts | Re-exports canonical ProviderError from provider module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (tx.body.referenceInputs && tx.body.referenceInputs.length > 0) { | ||
| referenceUtxos = yield* provider.Effect.getUtxosByOutRef(tx.body.referenceInputs).pipe( | ||
| Effect.mapError( | ||
| (e) => new WalletNew.WalletError({ message: `Failed to fetch reference UTxOs: ${e.message}`, cause: e }) | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| return yield* wallet.Effect.signTx(txOrHex, { ...context, referenceUtxos }) | ||
| }) |
There was a problem hiding this comment.
In signTxWithAutoFetch, reference UTxOs are always fetched (and then passed into wallet.Effect.signTx) when the transaction has referenceInputs. For CIP-30 ApiWallet this work is unnecessary and can cause signTx to fail if provider.getUtxosByOutRef fails, even though the wallet can still sign without reference UTxOs. Consider branching on wallet.type === "api" (or feature-detecting support for referenceUtxos) to skip the network fetch and call wallet.Effect.signTx(txOrHex, context) directly for API wallets.
| const useStakeKey = | ||
| typeof address === "string" && // RewardAddress is a branded string | ||
| derivation.stakeKey !== undefined | ||
| const sk = useStakeKey |
There was a problem hiding this comment.
signMessage decides to use the stake key whenever address is a string and derivation.stakeKey is defined. If a caller passes a reward address but the wallet was derived without a stake key (e.g. enterprise/private-key wallet without stakeKey), this currently falls back to signing with the payment key, producing a signature that won’t verify for the reward address per CIP-0008. Consider explicitly failing with WalletError when address is a reward address but derivation.stakeKey is missing.
| const useStakeKey = | |
| typeof address === "string" && // RewardAddress is a branded string | |
| derivation.stakeKey !== undefined | |
| const sk = useStakeKey | |
| const isRewardAddress = typeof address === "string" // RewardAddress is a branded string | |
| if (isRewardAddress && derivation.stakeKey === undefined) { | |
| throw new WalletNew.WalletError({ | |
| message: "Cannot sign message for reward address: wallet does not have a stake key" | |
| }) | |
| } | |
| const sk = isRewardAddress |
| const fetchPrimaryAddress = (): Promise<CoreAddress.Address> => { | ||
| if (!addressPromise) { | ||
| addressPromise = (async () => { | ||
| const used = await api.getUsedAddresses() | ||
| const unused = await api.getUnusedAddresses() | ||
| const addrStr = used[0] ?? unused[0] | ||
| if (!addrStr) throw new WalletNew.WalletError({ message: "Wallet API returned no addresses", cause: null }) | ||
| try { | ||
| return CoreAddress.fromBech32(addrStr) | ||
| } catch { | ||
| try { | ||
| return CoreAddress.fromHex(addrStr) | ||
| } catch (error) { | ||
| throw new WalletNew.WalletError({ | ||
| message: `Invalid address format from wallet: ${addrStr}`, | ||
| cause: error as Error | ||
| }) | ||
| } | ||
| } | ||
| })() | ||
| } | ||
| try { | ||
| cachedAddress = CoreAddress.fromBech32(addrStr) | ||
| } catch { | ||
| try { | ||
| cachedAddress = CoreAddress.fromHex(addrStr) | ||
| } catch (error) { | ||
| return yield* Effect.fail( | ||
| new WalletNew.WalletError({ | ||
| message: `Invalid address format from wallet: ${addrStr}`, | ||
| cause: error as Error | ||
| }) | ||
| ) | ||
| } | ||
| return addressPromise | ||
| } | ||
|
|
||
| const fetchPrimaryRewardAddress = (): Promise<CoreRewardAddress.RewardAddress | null> => { | ||
| if (!rewardAddressPromise) { | ||
| rewardAddressPromise = api | ||
| .getRewardAddresses() | ||
| .then((rewards) => (rewards[0] ? Schema.decodeSync(CoreRewardAddress.RewardAddress)(rewards[0]) : null)) | ||
| } | ||
| return cachedAddress | ||
| return rewardAddressPromise | ||
| } |
There was a problem hiding this comment.
The new Promise-based caches (addressPromise / rewardAddressPromise) become permanently “poisoned” on the first rejection (e.g. transient CIP-30 error, user not connected yet). Subsequent calls will reuse the rejected Promise and never retry. Consider resetting the cached promise back to null on rejection (e.g. addressPromise = fetch().catch(e => { addressPromise = null; throw e })) so callers can recover from transient failures.
Summary
Follow-up to #224. These fixes were identified during architectural review of the client wallet provider layer but were not included in the merged PR.
Issues Fixed
submitTxrouting split: Fixed spread order increateSigningClientreturn object soprovideralways wins overwalletforsubmitTxon both the promise and Effect surfaces.ProviderError: Removed deadProviderErrorclass fromClient.ts(with optional fields) that shadowed the canonicalProvider.ProviderError(required fields); re-exports the canonical one fromindex.ts.computeRequiredKeyHashesSyncnow handlesRegDrepCert,UnregDrepCert,UpdateDrepCert(viadrepCredential),AuthCommitteeHotCert,ResignCommitteeColdCert(viacommitteeColdCredential),PoolRegistration(viapoolParams.operator), andPoolRetirement(viapoolKeyHash).wallet: SigningWallet | AnyWallet: Narrowed the wallet-onlycreateClientoverload fromAnyWallet(which is equivalent toSigningWallet | ApiWallet | ReadOnlyWallet | WalletFactory) to the correctSigningWallet | WalletFactory.signMessagealways uses payment key: Dispatch on address type — reward addresses (RewardAddressbranded string) use the stake key per CIP-0008; payment addresses use the payment key.cip30Wallet.signTxsilently ignoresreferenceUtxos: NarrowedApiWalletEffect.signTxcontext type to excludereferenceUtxossince CIP-30 wallets cannot use reference inputs.cip30Walletaddress cache: Replaced mutable shared variables with lazy Promise-based caches (addressPromise/rewardAddressPromise) to eliminate the race condition.createSigningClient: Replaced...wallet.Effect, ...provider.Effectspreads with an explicit field-by-field object typed asSigningClientEffect; uses.bind()for method forwarding to avoid implicit-any parameters.Files Changed
packages/evolution/src/sdk/client/ClientImpl.tspackages/evolution/src/sdk/client/Client.tspackages/evolution/src/sdk/client/Wallets.tspackages/evolution/src/sdk/wallet/WalletNew.tspackages/evolution/src/index.ts