test(wallets): focused Wallet integration suite (src/tests/e2e)#1944
Open
albertoelias-crossmint wants to merge 1 commit into
Open
test(wallets): focused Wallet integration suite (src/tests/e2e)#1944albertoelias-crossmint wants to merge 1 commit into
albertoelias-crossmint wants to merge 1 commit into
Conversation
|
Contributor
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/wallets/src/tests/e2e/polling.test.ts:1373-1433
**Hardcoded call-count assertions are tightly coupled to internal constants**
The test pins the exact number of `getTransaction` calls at specific absolute timestamps (e.g., `expect(callsAt31s).toBe(24)` at t=31 s, `callsAt31s + 4` over the next 8 s). Any change to `STATUS_POLLING_INTERVAL_MS` (currently 500 ms), the 1.1× growth factor, the cap value, or the post-approval 1 s sleep will silently invalidate these counts and produce an opaque `Expected: 24, Received: N` failure with no indication of what changed. The math in the test is correct for the current constants, but the assertions don't carry any record of which constants they depend on. Consider embedding the relevant constant values as named variables in the test, or adding a comment block that lists the numbers the assertion depends on so a future failure is immediately diagnosable.
### Issue 2 of 3
packages/wallets/src/tests/e2e/device-init-and-recovery.test.ts:916-924
**`createMockDeviceKeyStorage` is duplicated verbatim across two test files**
An identical copy of `createMockDeviceKeyStorage` appears in both `device-init-and-recovery.test.ts` and `signer-wiring-and-device-usesigner.test.ts`. If the storage interface gains a new required method (or a mock default needs updating), both copies must be changed in sync. Extracting this factory into `test-helpers.ts` — alongside `createMockApiClient` and `createMockWallet` — would keep the two test files aligned automatically.
### Issue 3 of 3
packages/wallets/src/tests/e2e/use-signer-resolution.test.ts:2604
**`isZeroed` guard diverges between files**
`server-signer-hygiene.test.ts` defines `isZeroed` as `buf.length > 0 && buf.every(b => b === 0)`, while this file omits the length guard (`bytes.every(b => b === 0)`). For a zero-length `Uint8Array`, the second variant returns `true` (vacuous truth), so a bug that produces an empty buffer instead of a zeroed 32-byte one would go undetected here. Both files are currently only called with 32-byte buffers so there is no active failure, but the inconsistency is worth aligning.
Reviews (1): Last reviewed commit: "test(wallets): add Wallet-level integrat..." | Re-trigger Greptile |
A small, organized integration suite for the Wallet orchestration that unit tests can't cover (mocked ApiClient). 18 tests in 4 by-concern files: - wallet.initialization: initDefaultSigner auto-assembly ladder - wallet.recovery: preAuthIfNeeded single-flight + ensureAuthenticated - wallet.transactions: approval payload selection + onTransactionStart ordering - wallet.factory-rewrap: from() chain-subclass state carryover Distilled from the refactor's local characterization suite: only genuinely-unique, current-behavior orchestration kept; everything covered by colocated unit tests (resolver/descriptors/operation-poller) or wallet.test.ts was dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
albertoelias-crossmint
force-pushed
the
wallets/e2e-integration-tests
branch
from
aeec47c to
6014171
Compare
albertoelias-crossmint
changed the title
Contributor
|
Reviews (2): Last reviewed commit: "test(wallets): add focused Wallet integr..." | Re-trigger Greptile |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| ); | ||
| if (useApiKeySigner) { | ||
| // biome-ignore lint/suspicious/noExplicitAny: api-key config is valid on every chain | ||
| await wallet.useSigner({ type: "api-key" } as any); |
Contributor
There was a problem hiding this comment.
do we need these casts?
maxsch-xmint
approved these changes
Jun 24, 2026
| type: "external-wallet", | ||
| address: "0x123", | ||
| onSign, | ||
| } as unknown as SignerConfigForChain<"base-sepolia">); |
Contributor
There was a problem hiding this comment.
same, do we need these type of casts?
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.
A small, organized Wallet-orchestration integration suite at
packages/wallets/src/tests/e2e/— 18 tests in 4 by-concern files, covering behavior that emerges from theWalletwiring services together (mockedApiClient) and that the colocated unit tests can't reach:wallet.initialization.test.tsinitDefaultSignerauto-assembly ladder (0 / 1 / >1 delegated, fail-swallow)wallet.recovery.test.tspreAuthIfNeededsingle-flight recover +ensureAuthenticatedorderingwallet.transactions.test.tsonTransactionStartorderingwallet.factory-rewrap.test.tsfrom()chain-subclass state carryoverHow this was scoped
Distilled from the refactor's local characterization suite (was never committed). Each candidate was checked against existing coverage and dropped if redundant:
resolver.test.ts(unit).recover()fallback / mapAddressToKey / AuthRejected / resume → already inwallet.test.ts.useSignerper-type,addSigner/removeSigner, payload shapes →wallet.test.ts/descriptors.test.ts/chain-adapter.test.ts.Result: only genuinely-unique, current-behavior orchestration remains — no duplicate coverage, no resolver internals re-tested through a full
Wallet.Verification
18/18 green;
tscclean;biomeerror-gate exits 0. Test-only (nosrc/logic touched).🤖 Generated with Claude Code