Skip to content

fix: pass undefined instead of empty object for signOptions in ChainWalletStore - #42

Merged
pyramation merged 1 commit into
hyperweb-io:mainfrom
faneaatiku:patch-1
Oct 1, 2026
Merged

pyramation merged 1 commit into
hyperweb-io:mainfrom
faneaatiku:patch-1

Conversation

@faneaatiku

Copy link
Copy Markdown
Contributor

Fix ChainWalletStore.getOfflineSigner() passing {} instead of undefined as signOptions to cosmosWallet.signAmino() and cosmosWallet.signDirect().

Problem
CosmosWallet in core defines defaultSignOptions = { preferNoSetFee: true } and applies them via signOptions || this.defaultSignOptions. Since {} is truthy, the fallback never triggers and the defaults are silently dropped.

Without preferNoSetFee: true, Keplr overrides the application-provided fee with its own calculation using the chain's native denom. This breaks any app that pays fees in a non-native denomination.

Fix
Pass undefined instead of {} for both signAmino and signDirect so the defaults in core are respected.

…alletStore

Fix `ChainWalletStore.getOfflineSigner()` passing `{}` instead of `undefined` as `signOptions` to `cosmosWallet.signAmino()` and `cosmosWallet.signDirect()`.

Problem
CosmosWallet in core defines defaultSignOptions = { preferNoSetFee: true } and applies them via signOptions || this.defaultSignOptions. Since {} is truthy, the fallback never triggers and the defaults are silently dropped.

Without preferNoSetFee: true, Keplr overrides the application-provided fee with its own calculation using the chain's native denom. This breaks any app that pays fees in a non-native denomination.

Fix
Pass undefined instead of {} for both signAmino and signDirect so the defaults in core are respected.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for this fix — and sorry for the long wait. The change itself is correct: CosmosWallet.signAmino/signDirect fall back with signOptions || this.defaultSignOptions, so a truthy {} silently drops preferNoSetFee: true and Keplr overrides the app's fee. Passing undefined restores the defaults. I also grepped for other {} signOptions call sites; the only other one is wallets/ledger/src/cosmos.ts, which is Ledger's own signer and not affected.

One blocker: two existing unit tests still assert the old {} argument, so pnpm test fails on this branch (merged onto current main; main itself is green):

packages/store test: ● ChainWalletStore › Offline Signer › should handle signAmino calls
packages/store test: ● ChainWalletStore › Offline Signer › should handle signDirect calls
    - Expected  {}
    + Received  undefined

Fix: in packages/store/__tests__/chain-wallet-store.test.ts, change the 4th expected argument of both toHaveBeenCalledWith(...) assertions from {} to undefined. With that change the full suite passes (store: 176/176). A ready-made version of that 2-line test update is on branch devin/1790884011-pr42-fixup (commit ea669dc) if the maintainers prefer to land it alongside this PR.

Non-blocking follow-up idea for maintainers: making CosmosWallet merge options ({ ...this.defaultSignOptions, ...signOptions }) instead of || would stop any caller from accidentally dropping the defaults by passing a partial object.

Reviewed by Devin on behalf of @pyramation

@devin-ai-integration

Copy link
Copy Markdown

Correction to the review above: the ready-made test fix now lives in #46 (branch fix/pr42-sign-options-tests, this PR's commit + the 2-line assertion update). Either land this PR with the assertions changed to undefined, or merge #46 — same result.

pyramation added a commit that referenced this pull request Oct 1, 2026
fix(store): preserve CosmosWallet default signOptions in getOfflineSigner (#42 + test update)
@pyramation
pyramation merged commit 9f04e9c into hyperweb-io:main Oct 1, 2026
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.

2 participants