refactor: extracts tx signer into a dedicated service#2579
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a new tx-signer microservice (HTTP API, DI, batching, wallet, tests), integrates it into CI/CD, Docker, Helm and the API via a feature-flagged ExternalSignerHttpSdkService that the TxManagerService delegates to when enabled. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as API Service
participant Flags as FeatureFlagsService
participant TxMgr as TxManagerService
participant ExternalSdk as ExternalSignerHttpSdk
participant TxSigner as tx-signer Service
Client->>API: Send sign request
API->>Flags: isEnabled(EXTERNAL_TX_SIGNER)?
Flags-->>API: true/false
alt external signer enabled
API->>TxMgr: signAndBroadcastWithFundingWallet(messages)
TxMgr->>ExternalSdk: signAndBroadcastWithFundingWallet(messages)
ExternalSdk->>TxSigner: POST /v1/tx/funding (encoded messages)
TxSigner->>TxSigner: decode → sign → broadcast
TxSigner-->>ExternalSdk: IndexedTx
ExternalSdk-->>TxMgr: IndexedTx
TxMgr-->>API: IndexedTx
else feature disabled
API->>TxMgr: signAndBroadcastWithFundingWallet(messages)
TxMgr->>TxMgr: internal batch signing flow
TxMgr-->>API: IndexedTx
end
API-->>Client: return result
sequenceDiagram
participant Client
participant TxSigner as tx-signer Service
participant Wallet as Wallet Factory
participant Batch as BatchSigningClient
participant Stargate as SigningStargateClient
participant Chain as Blockchain
Client->>TxSigner: POST /v1/tx/derived (derivationIndex, messages)
TxSigner->>TxSigner: decode messages
TxSigner->>Wallet: create(derivationIndex)
Wallet-->>TxSigner: Wallet
TxSigner->>Batch: signAndBroadcast(messages, options)
Batch->>Stargate: getAccount / sign / estimateGas / broadcast
Stargate->>Chain: RPC calls
Chain-->>Batch: responses (account, gas, tx hash)
Batch->>Stargate: getTx (with retry)
Chain-->>Batch: IndexedTx
Batch-->>TxSigner: IndexedTx
TxSigner-->>Client: { code, hash, rawLog }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2579 +/- ##
==========================================
+ Coverage 50.44% 50.59% +0.14%
==========================================
Files 1029 1043 +14
Lines 29245 29356 +111
Branches 6610 6660 +50
==========================================
+ Hits 14753 14852 +99
- Misses 14084 14117 +33
+ Partials 408 387 -21
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Fix all issues with AI agents
In @.github/workflows/reusable-build-image.yml:
- Around line 108-110: The workflow uses registry variables like
TX_SIGNER_REGISTRY when writing to GITHUB_ENV (see the elif branch that sets
"registry=${{ vars.TX_SIGNER_REGISTRY }}" and "app=tx-signer"), but there is no
validation that these repository variables are defined; add an early validation
step in the workflow (before any build.sh invocation) that checks each required
vars.TX_SIGNER_REGISTRY, vars.API_REGISTRY, vars.WEB_REGISTRY,
vars.PROVIDER_PROXY_REGISTRY, vars.PROVIDER_CONSOLE_REGISTRY,
vars.STATS_WEB_REGISTRY, vars.NOTIFICATIONS_REGISTRY, and vars.INDEXER_REGISTRY
and fails the job with a clear error message if any are empty so the workflow
fails fast with a helpful message rather than passing an empty registry into
build.sh.
In @.github/workflows/tx-signer-release.yml:
- Around line 10-83: The workflow is missing explicit least-privilege
permissions for the release and beta deploy jobs causing CodeQL warnings; update
the jobs named "release", "deploy-beta-mainnet", and "deploy-beta-sandbox" to
declare explicit permissions instead of relying on defaults (mirror the minimal
scopes used in "build" and "deploy-prod-*" jobs), e.g., grant only the exact
rights needed (contents: write for release if creating releases, packages: write
if publishing images) and add actions: read for all deploy-* jobs to support gh
run list in the reusable deploy workflow; ensure you reference the job names
release, deploy-beta-mainnet, and deploy-beta-sandbox when adding these
permissions blocks.
In `@apps/api/src/billing/config/env.config.ts`:
- Line 40: Update the TX_SIGNER_BASE_URL env schema to be optional: change its
zod definition from TX_SIGNER_BASE_URL: z.string() to TX_SIGNER_BASE_URL:
z.string().optional() so deployments don't fail when the EXTERNAL_TX_SIGNER
feature is disabled; locate the constant/field named TX_SIGNER_BASE_URL in
env.config.ts and adjust its zod validator accordingly so code that reads
FEATURE_FLAGS_EXTERNAL_TX_SIGNER_DISABLED can run without requiring that URL.
In
`@apps/api/src/billing/services/external-signer-http-sdk/external-signer-http-sdk.service.ts`:
- Around line 71-91: The post method in ExternalSignerHttpSdkService lacks a
request timeout; fetch a timeout value from
billingConfigService.get("TX_SIGNER_HTTP_TIMEOUT_MS") (falling back to a
sensible default like 30000 ms), parse it to a number, and pass it to axios.post
via the timeout option so the request to the URL constructed with
TX_SIGNER_BASE_URL is bounded; ensure the timeout value is used in the axios
config object and preserved in error logging (TX_SIGNER_REQUEST_FAILED) so
timeouts surface correctly.
In `@apps/tx-signer/env/.env`:
- Line 1: The .env is missing required values validated by envSchema
(FUNDING_WALLET_MNEMONIC, OLD_MASTER_WALLET_MNEMONIC,
DERIVATION_WALLET_MNEMONIC, RPC_NODE_ENDPOINT); add a non-committed sample
and/or defaults: create a .env.local.sample containing placeholder mnemonic
variables and short instructions to copy into .env.local for local dev, and
ensure RPC_NODE_ENDPOINT has a sensible default in .env (or add
environment-specific files like .env.sandbox/.env.testnet) so tx-signer can
start; reference the envSchema and the exact variable names above when
implementing the sample and updating the documentation.
In `@apps/tx-signer/package.json`:
- Line 7: The package.json has mismatched entries: the "main" field
("server.js") and the "prod" script point at dist/server.js while tsup builds
./src/index.ts to dist/index.js; update package.json so the "main" field is
"dist/index.js" and the "prod" script runs node dist/index.js (or alternatively
change tsup's entry/output to produce dist/server.js) — edit the "main" property
and the "prod" script to consistently reference dist/index.js to match the tsup
entry ./src/index.ts.
In `@apps/tx-signer/src/lib/batch-signing-client/batch-signing-client.service.ts`:
- Around line 58-60: The hasPendingTransactions getter is wrong because
this.semaphore.nrWaiting() only reports waiting callers and misses active
acquisitions; do not use tryAcquire() (it would consume a permit). Add an
explicit counter (e.g., private activeBatches = 0) and update
hasPendingTransactions to return this.activeBatches > 0 ||
this.semaphore.nrWaiting() > 0; increment activeBatches immediately after
successfully acquiring the semaphore and decrement it in the finally/cleanup
block right after releasing the semaphore (update the methods that call
semaphore.acquire()/semaphore.release() accordingly) so the counter always
reflects in-flight batches.
In `@apps/tx-signer/src/lib/create-route/create-route.ts`:
- Around line 1-10: The createRoute wrapper currently only removes "security"
via a TypeScript cast, so at runtime createOpenApiRoute still receives
routeConfig.security; change createRoute to destructure routeConfig and exclude
security before forwarding (e.g., const { security, ...payload } = routeConfig)
and then call createOpenApiRoute(payload) so the security property is actually
omitted at runtime; update references to routeConfig, createRoute, and
createOpenApiRoute accordingly.
In
`@apps/tx-signer/src/lib/signing-stargate-client-factory/signing-stargate-client.factory.spec.ts`:
- Around line 10-34: The factory and test must be updated to handle async
creation: change createSigningStargateClientFactory's returned factory to be
async and update its type signature to return Promise<SigningStargateClient>
(where it calls SigningStargateClient.createWithSigner) and ensure the
implementation awaits the call to SigningStargateClient.createWithSigner; then
update the test (the factory variable in
signing-stargate-client.factory.spec.ts) to call await on factory(endpoint,
signer) (mark the test async) and adjust expectations to work with the resolved
mock Promise (ensure mockFactory is mocked to return a Promise or the test
awaits the returned Promise before asserting result equals mockClient).
In `@apps/tx-signer/src/lib/wallet/wallet.ts`:
- Around line 53-56: The getFirstAddress method currently assumes getAccounts()
returns a non-empty array which will throw when accessing accounts[0].address;
update getFirstAddress to defensively check the result of this.getAccounts()
(call to getAccounts) and if the array is empty either throw a clear error
(e.g., "No accounts available") or handle the empty case explicitly before
returning accounts[0].address so callers won't hit a runtime exception.
In `@apps/tx-signer/src/providers/type-registry.provider.ts`:
- Around line 19-40: Add a shared type guard (e.g., function isGeneratedType(x):
x is { $type: string } that returns Boolean(x) && typeof x === "object" &&
"$type" in x) and use it to filter inputs before .map() for
defaultRegistryTypes, akashTypes, and newAkashTypes so the 'in' operator never
runs on primitives; update defaultRegistryTypes' .filter(x => x && "$type" in x)
to .filter(isGeneratedType), insert .filter(isGeneratedType) before mapping
akashTypes (which currently lacks any filter), and replace newAkashTypes'
.filter(x => "$type" in x) with .filter(isGeneratedType) to prevent undefined
entries and runtime errors.
In `@apps/tx-signer/src/services/config/config.service.ts`:
- Around line 3-8: The generic default for C is causing inferred schema
properties to collapse to never; update both the ConfigServiceOptions interface
and the ConfigService class to use an empty object type as the default for C
(replace Record<string, never> with {}), so that the private readonly config: C
& z.infer<E> and methods like get() preserve the inferred z.infer<E> types when
C is omitted.
In `@apps/tx-signer/src/services/shutdown-server/shutdown-server.spec.ts`:
- Around line 7-17: Refactor the test to use a local setup helper that creates
and returns fresh mocks and the function under test instead of inlining them:
create a setup() function at the bottom of the describe that constructs the
mocked ServerType (with listening: false), a mocked Logger, and returns {
server, logger }; then update the test to call const { server, logger } =
setup() and use those values, and ensure no shared state or beforeEach is used;
reference the shutdownServer function and the ServerType and Logger mocks when
implementing the helper.
In `@apps/tx-signer/src/services/start-server/start-server.ts`:
- Around line 49-52: The process exit handler uses the "exit" event which won't
wait for async cleanup in shutdown() (which calls shutdownServer()); change the
listener from processEvents.on("exit", ...) to processEvents.on("beforeExit",
async (code) => { await shutdown(`BEFOREEXIT:${code}`); }) (or equivalent) so
the async shutdown() is awaited and cleanup completes; keep the existing
SIGINT/SIGTERM handlers that call shutdown("SIGINT"/"SIGTERM") and ensure you
reference the shutdown and shutdownServer functions in the updated handler.
In `@apps/tx-signer/test/functional/sign-and-broadcast-tx.spec.ts`:
- Around line 42-61: Tests register a mocked TxManagerService directly into the
DI container which can leak across tests; call container.clearInstances() at the
start of this test (or move the registration into a shared setup function) to
ensure a clean container before registering the mock. Specifically, before
registering the mock TxManagerService and calling
container.registerInstance(TxManagerService, txManagerService), invoke
container.clearInstances() (or refactor the mock/registration into the test
suite's setup helper) so signAndBroadcastWithDerivedWallet and other mocks don't
pollute subsequent tests.
🧹 Nitpick comments (30)
apps/tx-signer/src/caching/helpers/helpers.spec.ts (1)
3-26: Consider adding a test for argument-based memoization.The current tests verify memoization for no-argument calls and rejection handling. Consider adding a test that verifies different arguments result in separate cache entries (i.e.,
memoized('a')andmemoized('b')should callfntwice, while repeated calls with'a'should use the cache).💡 Suggested test case
+ it("caches results per unique arguments", async () => { + const fn = jest.fn().mockImplementation((arg: string) => Promise.resolve(arg)); + const memoized = memoizeAsync(fn); + + const result1 = await memoized("a"); + const result2 = await memoized("b"); + const result3 = await memoized("a"); + + expect(result1).toBe("a"); + expect(result2).toBe("b"); + expect(result3).toBe("a"); + expect(fn).toHaveBeenCalledTimes(2); + });apps/deploy-web/public/sdl-schema.yaml (2)
60-81: Description mentions enforcement not implemented in schema.The description states "Non-RAM classes (beta1, beta2, beta3, default) require persistent=true", but the schema only enforces that RAM class must NOT have
persistent=true. The requirement for non-RAM classes to havepersistent=trueisn't validated here.This may be intentional given the note about semantic validations at runtime, but consider updating the description to clarify what this schema definition actually validates versus what's deferred to runtime.
📝 Suggested description clarification
storageAttributesValidation: description: | Storage attributes validation: - 1. RAM class must not be persistent - 2. Non-RAM classes (beta1, beta2, beta3, default) require persistent=true + Validates that RAM class must not have persistent=true. + Note: The requirement for non-RAM classes to have persistent=true + is enforced at runtime by the Go parser.
184-222: GPU bidirectional validation not fully enforced in schema.The description mentions bidirectional validation: "units > 0 requires attributes, and attributes require units > 0". However, the schema only requires
unitsbut doesn't enforce:
- If
units > 0, thenattributesmust be present- If
attributesis present, thenunitsmust be> 0If this is intentionally deferred to runtime validation (consistent with the top-level note), consider clarifying in the description. Otherwise, conditional schemas (
if/then) could enforce this.apps/tx-signer/env/.env.sample (1)
1-13: Sample file may be missing some environment variables.Comparing with
.env.functional.testand.env.unit.test, this sample appears to be missing:
NETWORKREST_API_NODE_URLLOG_LEVELDEPLOYMENT_ENVSTD_OUT_LOG_FORMATConsider adding these placeholders to help developers understand the full configuration surface.
apps/tx-signer/src/lib/create-route/create-route.spec.ts (1)
1-17: Consider coveringsecuritystripping to prevent regressions.
A small test asserting the returned route omitssecuritywould lock in the intended behavior once the runtime strip is applied.✅ Example add-on test
describe(createRoute.name, () => { it("returns a route config with path", () => { const route = createRoute({ method: "get", path: "/test", responses: { description: "ok" } } }); expect(route.path).toBe("/test"); }); + + it("omits security in the returned route config", () => { + const route = createRoute({ + method: "get", + path: "/secure", + security: [{ bearerAuth: [] }], + responses: { + 200: { description: "ok" } + } + }); + + expect("security" in route).toBe(false); + }); });packages/docker/docker-compose.prod-with-db.yml (1)
3-7: Add healthcheck to tx-signer service and change depends_on condition to service_healthy.tx-signer exposes a
/healthzendpoint but docker-compose.prod-with-db.yml doesn't define a healthcheck for it. Currently usingservice_startedonly confirms the container is up, not that the signer is ready. Add a healthcheck configuration (similar to the db service in the same file) and change the condition toservice_healthyto ensure the API doesn't race against an unready signer.apps/tx-signer/src/services/start-server/start-server.spec.ts (1)
8-18: Consider using asetupfunction pattern per coding guidelines.The test should use a
setupfunction at the bottom of the describe block instead of inline object creation. This improves reusability and follows project conventions.♻️ Suggested refactor with setup function
describe(startServer.name, () => { it("starts and returns a server", async () => { - const app = new Hono(); - const logger = mock<LoggerService>(); - const emitter = new EventEmitter(); - - const server = await startServer(app, logger, emitter, { port: 0 }); + const { server } = await setup(); expect(server).toBeDefined(); await new Promise<void>(resolve => server?.close(() => resolve())); }); + + async function setup() { + const app = new Hono(); + const logger = mock<LoggerService>(); + const emitter = new EventEmitter(); + const server = await startServer(app, logger, emitter, { port: 0 }); + return { app, logger, emitter, server }; + } });As per coding guidelines: "Use
setupfunction instead ofbeforeEachin test files."apps/tx-signer/src/http-schemas/tx-signer/tx-signer.schema.ts (1)
14-26: Consider adding integer and non-negative validation forderivationIndex.The
derivationIndexis used for HD wallet path derivation and should be a non-negative integer. Usingz.number()allows floats and negative values.♻️ Suggested validation improvement
export const SignAndBroadcastDerivedRequestInputSchema = z.object({ data: z.object({ - derivationIndex: z.number(), + derivationIndex: z.number().int().nonnegative(), messages: z.array(EncodedMessageSchema).min(1), options: z .object({apps/tx-signer/src/http-schemas/tx-signer/tx-signer.schema.spec.ts (2)
3-3: Use a named reference instead of a hardcoded string in describe block.Per coding guidelines, use a reference like
SignAndBroadcastFundingRequestInputSchema.descriptionor create a module-level constant for the describe name to enable automated refactoring tools.
3-17: Consider expanding test coverage for schema validation.The test only covers the happy path for
SignAndBroadcastFundingRequestInputSchema. Consider adding tests for:
- Invalid payloads (missing fields, empty messages array)
SignAndBroadcastDerivedRequestInputSchemavalidationSignAndBroadcastResponseOutputSchemavalidationapps/tx-signer/src/services/hono-error-handler/hono-error-handler.service.ts (2)
85-88: Minor: HTTP 502 maps to "service_unavailable" which is technically "Bad Gateway".Both 502 (Bad Gateway) and 503 (Service Unavailable) return the same code "service_unavailable". Consider using "bad_gateway" for 502 if semantic precision is desired.
♻️ Optional: Differentiate 502 and 503
case 502: - return "service_unavailable"; + return "bad_gateway"; case 503: return "service_unavailable";
71-91: Missing case for HTTP 500 ingetErrorCode.The switch handles 400, 401, 403, 404, 409, 429, 502, 503 but not 500. A 500 error would fall through to "unknown_error" rather than a more appropriate "internal_server_error" code.
♻️ Add 500 case
case 429: return "rate_limited"; + case 500: + return "internal_server_error"; case 502: - return "service_unavailable"; + return "bad_gateway";apps/tx-signer/src/lib/wallet/wallet.spec.ts (1)
3-9: Test structure follows guidelines, but coverage is minimal.The test correctly uses
Wallet.namein the describe block and validates the core address prefix behavior. Consider expanding coverage to test:
- Mnemonic-based wallet creation (deterministic addresses)
- Index-based HD path derivation
signDirectandsignAminomethodsapps/tx-signer/src/lib/batch-signing-client/batch-signing-client.service.ts (2)
132-162: Signing batch continues after first failure, potentially wasting gas simulations.When a signing error occurs (line 157), subsequent transactions in the batch continue to be processed. However, if the error is due to a fundamental issue (e.g., account not found, insufficient funds), continuing to sign may waste RPC calls for gas estimation.
This is acceptable if partial batch success is the intended behavior, but consider whether early termination on certain error types would be more efficient.
164-200: Sequential broadcast loop is correct but could be simplified.The while loop with manual index management works but could be more idiomatic using a for-of loop with index tracking or
entries().Optional simplification
private async broadcastBatch(signResults: Result<TxRaw, unknown>[]): Promise<Result<string, unknown>[]> { const results: Result<string, unknown>[] = []; - let index = 0; - - while (index < signResults.length) { - const signResult = signResults[index]; + for (const [index, signResult] of signResults.entries()) { if (!signResult.ok) { results.push(signResult); - index++; continue; } // ... rest of the logic - index++; } return results; }apps/tx-signer/src/lib/batch-signing-client/batch-signing-client.service.spec.ts (7)
24-36: Test descriptions should not start with "should".Per coding guidelines, test descriptions should use present simple, 3rd person singular without prepending "should".
Proposed fix
- it("should batch and execute multiple transactions successfully", async () => { + it("batches and executes multiple transactions successfully", async () => {As per coding guidelines: Use present simple, 3rd person singular for test descriptions without prepending 'should'.
38-57: Test description should follow guidelines.Proposed fix
- it("should handle errors in batch without affecting other transactions", async () => { + it("handles errors in batch without affecting other transactions", async () => {As per coding guidelines: Use present simple, 3rd person singular for test descriptions without prepending 'should'.
59-81: Test description should follow guidelines.Proposed fix
- it("should retry failed transaction within a batch on sequence mismatch error and eventually succeed", async () => { + it("retries failed transaction within a batch on sequence mismatch error and eventually succeeds", async () => {As per coding guidelines: Use present simple, 3rd person singular for test descriptions without prepending 'should'.
83-97: Test description should follow guidelines.Proposed fix
- it("should recover transaction when getTx fails with network error but tx exists on chain", async () => { + it("recovers transaction when getTx fails with network error but tx exists on chain", async () => {As per coding guidelines: Use present simple, 3rd person singular for test descriptions without prepending 'should'.
99-113: Test description should follow guidelines.Proposed fix
- it("should recover transaction when getTx fails with socket error", async () => { + it("recovers transaction when getTx fails with socket error", async () => {As per coding guidelines: Use present simple, 3rd person singular for test descriptions without prepending 'should'.
115-131: Test description should follow guidelines.Proposed fix
- it("should recover transaction when getTx fails with cosmjs fetch failed error with cause", async () => { + it("recovers transaction when getTx fails with cosmjs fetch failed error with cause", async () => {As per coding guidelines: Use present simple, 3rd person singular for test descriptions without prepending 'should'.
133-144: Test description should follow guidelines.Proposed fix
- it("should not attempt recovery for non-network errors", async () => { + it("does not attempt recovery for non-network errors", async () => {As per coding guidelines: Use present simple, 3rd person singular for test descriptions without prepending 'should'.
apps/tx-signer/src/services/tx-manager/tx-manager.service.ts (1)
104-115: Client cleanup logic may have a race condition.The check
!client.hasPendingTransactionshappens aftersignAndBroadcastcompletes. If another request arrives between the broadcast completion and the cleanup check, and that request uses the same derived wallet, the client might be deleted while the new request is using it.However, since
#getClientwill recreate the client if missing, this is safe but may cause unnecessary client recreation. This is likely acceptable given the low probability and self-healing nature.apps/tx-signer/src/services/hono-error-handler/hono-error-handler.service.spec.ts (1)
6-15: Use asetupfunction instead of direct instantiation.Per coding guidelines, tests should use a
setupfunction at the bottom of the describe block instead of inline instantiation.♻️ Suggested refactor
describe(HonoErrorHandlerService.name, () => { it("returns 500 response for unknown errors", async () => { - const service = new HonoErrorHandlerService(); - const context = mock<AppContext>({ - json: ((body: unknown, init: ResponseInit) => new Response(JSON.stringify(body), init)) as AppContext["json"] - }); + const { service, context } = setup(); const response = await service.handle(new Error("boom"), context); expect(response.status).toBe(500); }); + + function setup() { + const context = mock<AppContext>({ + json: ((body: unknown, init: ResponseInit) => new Response(JSON.stringify(body), init)) as AppContext["json"] + }); + const service = new HonoErrorHandlerService(); + return { service, context }; + } });As per coding guidelines: "Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock."apps/tx-signer/src/routes/tx/tx.router.spec.ts (1)
7-7: UsetxRouter.constructor.nameor a reference instead of hardcoded string.Per coding guidelines, use a dynamic reference for describe suite descriptions to enable automated refactoring tools.
However, since
txRouteris an instance ofOpenApiHonoHandler, usingtxRouter.constructor.namewould yield "OpenApiHonoHandler" which may not be descriptive. Consider keeping the current approach or using a constant.apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
30-32: Config-driven disable takes precedence overENABLE_ALL.The
EXTERNAL_TX_SIGNER_DISABLEDcheck runs beforeENABLE_ALL, so this flag can be disabled even when all feature flags are globally enabled. If this is intentional (e.g., for safety in test environments), consider adding a brief inline comment to clarify the precedence.apps/tx-signer/src/controllers/tx-manager/tx-manager.controller.spec.ts (1)
7-32: Consider using thesetupfunction pattern per coding guidelines.Per guidelines, tests should use a
setupfunction at the bottom of the describe block. This keeps instantiation consistent and simplifies adding more test cases (e.g., forsignWithDerivedWallet).♻️ Suggested refactor
describe(TxManagerController.name, () => { it("decodes messages and signs with funding wallet", async () => { - const decodedMessage = { typeUrl: "/test.MsgTest", value: { foo: "bar" } }; - const registry = mock<Registry>({ - decode: jest.fn().mockReturnValue(decodedMessage.value) - }); - const txManagerService = mock<TxManagerService>({ - signAndBroadcastWithFundingWallet: jest.fn().mockResolvedValue({ code: 0, hash: "tx", rawLog: "" }) - }); - - const controller = new TxManagerController(registry, txManagerService); + const decodedMessage = { typeUrl: "/test.MsgTest", value: { foo: "bar" } }; + const { controller, registry, txManagerService } = setup({ + decodeResult: decodedMessage.value, + signResult: { code: 0, hash: "tx", rawLog: "" } + }); + const result = await controller.signWithFundingWallet({ data: { messages: [ { typeUrl: decodedMessage.typeUrl, value: Buffer.from([1, 2, 3]).toString("base64") } ] } }); expect(registry.decode).toHaveBeenCalledWith({ typeUrl: decodedMessage.typeUrl, value: expect.any(Uint8Array) }); expect(txManagerService.signAndBroadcastWithFundingWallet).toHaveBeenCalledWith([decodedMessage]); expect(result.data.code).toBe(0); }); + + function setup({ decodeResult, signResult }: { decodeResult?: unknown; signResult?: { code: number; hash: string; rawLog: string } }) { + const registry = mock<Registry>({ + decode: jest.fn().mockReturnValue(decodeResult) + }); + const txManagerService = mock<TxManagerService>({ + signAndBroadcastWithFundingWallet: jest.fn().mockResolvedValue(signResult) + }); + const controller = new TxManagerController(registry, txManagerService); + return { controller, registry, txManagerService }; + } });As per coding guidelines: "Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock."apps/tx-signer/src/routes/healthz/healthz.router.ts (1)
28-31: Consider resolving the controller once if it's stateless.The controller is resolved from the DI container on every request. If
HealthzControlleris stateless (registered as singleton), resolving once at module load could avoid repeated lookups:const controller = container.resolve(HealthzController); healthzRouter.openapi(healthzRoute, async c => { const payload = await controller.getStatus(); return c.json(payload, 200); });However, the current approach is acceptable if the controller needs request-scoped dependencies or if consistency with other routers is preferred.
apps/tx-signer/src/routes/tx/tx.router.ts (1)
14-43: Consider documenting error responses in the OpenAPI spec.The route definitions only specify the
200success response. For a complete OpenAPI spec, consider adding common error responses (e.g.,400for validation errors,500for server errors) if they're not handled by a global error schema.apps/tx-signer/package.json (1)
52-52: Consider pinning zod to a more specific version range.The
"3.*"version range is quite loose and could lead to unexpected breaking changes. Consider using a more specific range like"^3.23.0"or aligning with other packages in the monorepo.
apps/api/src/billing/services/external-signer-http-sdk/external-signer-http-sdk.service.ts
Show resolved
Hide resolved
ygrishajev
force-pushed
the
feature/tx-signer
branch
4 times, most recently
from
6f15578 to
13f0e97
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/docker/.env.sandbox.docker-compose-dev (1)
2-8: Fix dotenv-linter UnorderedKey by moving TX_SIGNER_BASE_URL earlier.Line 8 triggers the UnorderedKey warning; reordering keeps lint clean.
🔧 Proposed reorder
AkashSandboxDatabaseCS=postgres://postgres:password@db:5432/console-akash-sandbox +TX_SIGNER_BASE_URL=http://tx-signer:3000 UserDatabaseCS=postgres://postgres:password@db:5432/console-users NETWORK=sandbox POSTGRES_DB_URI=postgres://postgres:password@db:5432/console-users DB_HOST=db NOTIFICATIONS_API_BASE_URL=http://notifications:3000 -TX_SIGNER_BASE_URL=http://tx-signer:3000
🤖 Fix all issues with AI agents
In `@apps/api/src/core/services/start-server/start-server.ts`:
- Around line 56-59: The current use of processEvents.on("beforeExit", ...) is
inappropriate for process.exit/uncaught errors; update the shutdown wiring by
replacing the beforeExit listener with an "exit" listener or remove it
altogether, and add handlers for "uncaughtException" and "unhandledRejection"
that call shutdown, while keeping the existing processEvents.on("SIGTERM", ...)
and processEvents.on("SIGINT", ...) hooks; target the processEvents.on calls and
the shutdown function to implement this change.
In `@apps/api/src/server.ts`:
- Around line 50-60: Replace the dead child.once("beforeExit", ...) listener
with child.once("exit", ...) so non‑zero child exits are correctly detected and
rejected; update the handler attached to the ChildProcess instance (the code
using child.once and reject with INTERFACE in the error message) to listen for
"exit" (or "close" if you prefer) instead of "beforeExit", and ensure the
disconnect cleanup logic (disconnect function and process.on handlers) remains
unchanged.
In `@apps/tx-signer/env/.env.functional.test`:
- Around line 1-6: Reorder the environment variable keys in the
.env.functional.test file to satisfy dotenv-linter (e.g., sort keys
alphabetically); specifically ensure keys like DEPLOYMENT_ENV, LOG_LEVEL,
NETWORK, REST_API_NODE_URL, RPC_NODE_ENDPOINT, STD_OUT_LOG_FORMAT are placed in
the linter-expected order (alphabetical or project-specified ordering) so the
dotenv-linter no longer flags the file.
In `@apps/tx-signer/env/.env.mainnet`:
- Around line 1-2: The file is missing a trailing newline which trips
dotenv-linter; open the env file containing REST_API_NODE_URL and
RPC_NODE_ENDPOINT and add a single final newline (EOF newline) so the file ends
with a newline character, then save and re-run lint/CI to confirm the warning is
resolved.
In `@apps/tx-signer/env/.env.sample`:
- Around line 1-13: The .env.sample triggers dotenv-linter UnorderedKey warnings
because the environment keys are not sorted; fix this by alphabetizing all keys
(e.g., AVERAGE_GAS_PRICE, DERIVATION_WALLET_MNEMONIC,
DERIVATION_WALLET_MNEMONIC_V1, DERIVATION_WALLET_MNEMONIC_V2,
FUNDING_WALLET_MNEMONIC, FUNDING_WALLET_MNEMONIC_V1, FUNDING_WALLET_MNEMONIC_V2,
GAS_SAFETY_MULTIPLIER, OLD_MASTER_WALLET_MNEMONIC, PORT, RPC_NODE_ENDPOINT,
SERVER_ORIGIN, WALLET_BATCHING_INTERVAL_MS), preserving the same key names and
blank-line/formatting conventions so dotenv-linter no longer reports
UnorderedKey.
In `@apps/tx-signer/env/.env.unit.test`:
- Around line 1-10: This file contains plaintext wallet mnemonics
(OLD_MASTER_WALLET_MNEMONIC, FUNDING_WALLET_MNEMONIC,
DERIVATION_WALLET_MNEMONIC); remove the secrets from env/.env.unit.test and
replace their values with placeholder variables (e.g., "<REPLACE_WITH_SECRET>")
or omit them, commit an .env.unit.test.example with those placeholders, and
update CI/local docs to load the real mnemonics from CI secrets or a local .env
(set via environment variables in your pipeline) so the actual mnemonic values
are never committed.
- Around line 1-10: The dotenv file keys are out of order and causing
dotenv-linter failures; reorder the environment variables (NETWORK,
OLD_MASTER_WALLET_MNEMONIC, FUNDING_WALLET_MNEMONIC, DERIVATION_WALLET_MNEMONIC,
REST_API_NODE_URL, RPC_NODE_ENDPOINT, LOG_LEVEL, STD_OUT_LOG_FORMAT,
SQL_LOG_FORMAT, DEPLOYMENT_ENV) into the order expected by dotenv-linter
(typically alphabetical by key) so the linter passes.
In
`@apps/tx-signer/src/lib/signing-stargate-client-factory/signing-stargate-client.factory.ts`:
- Around line 8-25: The returned factory must be async and await the Comet38
client creation: change CreateSigningStargateClient to return
Promise<SigningStargateClient>, make createSigningStargateClientFactory's inner
function async, call and await ProvidedComet38Client.create(client) (referencing
ProvidedComet38Client.create) and then await/return the result of
factory(cometClient, signer, options) (referencing
SigningStargateClient.createWithSigner) so you pass an actual Comet38Client
instance rather than a Promise.
In `@apps/tx-signer/src/services/config/config.service.ts`:
- Around line 10-14: The constructor currently spreads an optional
options.config into this.config allowing an unsafe {} cast; update the
ConfigService constructor to validate options.config at runtime (in the
constructor of class ConfigService) and either throw a clear error if
options.config is undefined or merge in a safe default before assigning to
this.config (preserving the C & z.infer<E> type). Specifically, check
options.config (from ConfigServiceOptions<E,C>) and handle the undefined case
instead of blindly doing this.config = { ...options.config } as C & z.infer<E>,
so callers won’t get a fake empty object at runtime.
In `@apps/tx-signer/src/services/tx-manager/tx-manager.service.ts`:
- Around line 8-9: Remove the blanket eslint suppression before the import of
LoggerService: delete the "// eslint-disable-next-line" comment preceding
"import { LoggerService } ..." in tx-manager.service.ts so linting rules apply
normally; if a specific rule genuinely needs disabling, replace the blanket
suppression with a targeted rule (e.g., // eslint-disable-next-line <rule-name>)
next to the exact offending line after validating why it's required.
In `@packages/docker/docker-compose.prod-with-db.yml`:
- Around line 6-7: The current dependency uses "condition: service_started" for
the tx-signer service; replace it with "condition: service_healthy" and add a
corresponding HEALTHCHECK to the tx-signer service definition (in
docker-compose.prod.yml where tx-signer is defined) that tests the signer’s API
readiness (e.g., curl or a small probe against the signer’s health endpoint);
alternatively, if you determine "service_started" is sufficient, add a comment
in docker-compose.prod-with-db.yml documenting that decision and why readiness
is not required.
🧹 Nitpick comments (6)
apps/tx-signer/src/services/tracing/tracing.service.ts (1)
7-27: Missing context propagation breaks parent-child span relationships.The function doesn't set the span as the active context during
fn()execution. This means nested spans created withinfn()won't automatically be linked as children of this span, breaking distributed tracing hierarchies.Additionally, for consistency with existing patterns in
job-queue.service.ts, consider recording exceptions for non-Error throws as well.♻️ Proposed fix with context propagation and consistent exception recording
-import { SpanStatusCode, trace } from "@opentelemetry/api"; +import { context, SpanStatusCode, trace } from "@opentelemetry/api"; export function createSpan(name: string) { return trace.getTracer("default").startSpan(name); } export async function withSpan<T>(spanName: string, fn: () => Promise<T>): Promise<T> { const span = createSpan(spanName); + const ctx = trace.setSpan(context.active(), span); try { - const result = await fn(); + const result = await context.with(ctx, fn); span.setStatus({ code: SpanStatusCode.OK }); return result; } catch (error: unknown) { const message = error instanceof Error ? error.message : "Unknown error"; span.setStatus({ code: SpanStatusCode.ERROR, message }); - if (error instanceof Error) { - span.recordException(error); - } + span.recordException(error instanceof Error ? error : new Error(String(error))); throw error; } finally { span.end(); } }apps/tx-signer/env/.env.staging (1)
1-1: Port 80 binding is supported via container capabilities; verify staging environment intent.The Dockerfile.node production stage grants
cap_net_bind_serviceto the node executable before switching to the non-rootappuser, enabling privileged port binding. However,PORT=80differs from production (PORT: 3000) and the default config (PORT: 3091). Confirm this staging-specific port assignment is intentional.apps/tx-signer/src/lib/create-route/create-route.spec.ts (1)
3-16: Consider adding test coverage for security stripping behavior.The test verifies path propagation, but given that
createRouteis designed to handle thesecurityproperty specially, consider adding a test case that verifies security is properly handled (stripped or ignored) when provided in the input config.apps/tx-signer/src/providers/raw-app-config.provider.ts (1)
3-3: Type includesnumberbutprocess.envvalues are always strings.
process.envvalues are eitherstringorundefined, nevernumber. Consider usingRecord<string, string | undefined>for type accuracy, or if the broader type is intentional for flexibility, this is fine as-is.apps/tx-signer/src/http-schemas/tx-signer/tx-signer.schema.ts (1)
14-17: HardenderivationIndexto an integer, non‑negative value.
Fractional/negative indexes are invalid for HD wallet derivation and should be rejected early.♻️ Proposed change
- derivationIndex: z.number(), + derivationIndex: z.number().int().nonnegative(),apps/tx-signer/src/lib/batch-signing-client/batch-signing-client.service.ts (1)
176-212: Clarify the broadcast strategy: sync for all but last, async for final.The broadcast logic uses
broadcastTxSyncfor intermediate transactions andbroadcastTx(async/confirmed) for the last one. This is an optimization to reduce latency, but it means intermediate transactions may not be confirmed when the batch returns. Consider adding a brief inline comment to document this intentional design choice for future maintainers.
apps/tx-signer/src/lib/signing-stargate-client-factory/signing-stargate-client.factory.ts
Show resolved
Hide resolved
ygrishajev
force-pushed
the
feature/tx-signer
branch
3 times, most recently
from
fd07a40 to
eb3fff9
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
`@apps/api/src/billing/services/external-signer-http-sdk/external-signer-http-sdk.service.ts`:
- Around line 30-62: The response type SignAndBroadcastResponse currently uses
IndexedTx but the tx-signer returns only the subset { code, hash, rawLog } (per
SignAndBroadcastResponseOutputSchema); update the contract by replacing
SignAndBroadcastResponse.data with a narrow type matching the actual schema
(e.g., an interface with code, hash, rawLog) and update usages in
ExternalSignerHttpSdkService.signAndBroadcastWithFundingWallet and
signAndBroadcastWithDerivedWallet (and the generic post<T> calls) to use the new
type, or alternatively modify the tx-signer output to return a full IndexedTx if
you want callers to receive all fields—ensure the type used in the post<>
generic matches the actual runtime shape.
In `@apps/tx-signer/src/http-schemas/tx-signer/tx-signer.schema.ts`:
- Around line 14-26: The SignAndBroadcastDerivedRequestInputSchema currently
allows any number for derivationIndex; update the derivationIndex validator to
enforce a non-negative integer by replacing z.number() with
z.number().int().nonnegative() (i.e., change the derivationIndex schema inside
SignAndBroadcastDerivedRequestInputSchema to use .int().nonnegative()) so floats
and negative values are rejected.
In
`@apps/tx-signer/src/services/hono-error-handler/hono-error-handler.service.ts`:
- Around line 32-44: The handler currently accesses error.data without type
safety; update the isHttpError block in hono-error-handler.service (the branch
using isHttpError, getErrorCode, getErrorType and c.json) to only include data
when present or use a narrowed type - e.g., define a HttpErrorWithData =
HttpError & { data?: unknown } and cast/narrow error to that before reading
data, or check "if ('data' in error && error.data !== undefined)" and include it
conditionally in the response object passed to c.json so you never serialize an
undefined property.
In `@apps/tx-signer/src/services/tracing/tracing.service.ts`:
- Around line 7-27: withSpan creates a span but doesn't make it the active span,
so child spans inside fn() aren't parented; update withSpan to set the created
span into the active context (using trace.setSpan(context.active(), span)) and
execute fn() inside context.with(...) so context propagation matches
job-queue.service.ts; also ensure exceptions that are not Error objects are
recorded by converting them (e.g., recordException(new Error(String(error))) or
similar) and include that message when calling span.setStatus.
In `@apps/tx-signer/src/services/tx-manager/tx-manager.service.ts`:
- Around line 121-134: The `#getClient` method can create duplicate clients under
concurrency because two callers can pass the has(address) check; fix by caching
the in-flight creation promise in `#clientsByAddress`: after deriving address (via
getDerivedWallet and wallet.getFirstAddress) immediately set
`#clientsByAddress.set`(address, { address, client: <Promise> }) where client is
the Promise returned by invoking batchSigningClientServiceFactory(wallet) (store
the Promise synchronously before awaiting), then await that promise and return
the resolved CachedClient; alternatively use a mutex keyed by address to ensure
only one creation runs (key methods: `#getClient`, `#clientsByAddress`,
batchSigningClientServiceFactory).
In `@apps/tx-signer/test/setup-unit-tests.ts`:
- Around line 3-8: The afterAll teardown currently swallows all errors from
container.dispose(); update the afterAll to avoid a blank catch by either
checking and using an idempotent guard (e.g., if (!container.isDisposed) await
container.dispose()) or by catching only expected/benign errors and rethrowing
others; specifically modify the afterAll block that calls container.dispose() so
it does not use an empty catch, instead detect known disposable-state errors or
rethrow unexpected exceptions from container.dispose() to surface real failures.
🧹 Nitpick comments (6)
packages/docker/docker-compose.prod.yml (1)
10-20: Newtx-signerservice looks good overall.The service configuration is consistent with other services in the file. A couple of minor observations:
Quote style inconsistency: The
tx-signerandapiservices now use double quotes for port mappings, whilenotifications(line 29) and others still use single quotes. Consider aligning for consistency.Environment configuration: Verify that the
tx-signerservice doesn't require additional environment variables (e.g., for connecting to the blockchain RPC, wallet configuration, or API endpoints) beyond what's provided in.env.sandbox.docker-compose-dev.apps/tx-signer/src/services/shutdown-server/shutdown-server.spec.ts (1)
7-17: Consider adding test coverage for remaining code paths.The test only covers the "not listening" scenario. The
shutdownServerfunction has multiple branches that would benefit from coverage:
- Server is listening →
server.close()is calledonShutdowncallback is invoked and completes successfullyonShutdowncallback throws → logsON_SHUTDOWN_ERRORserver.close()fails → logsSERVER_CLOSE_ERRORapps/tx-signer/src/services/hono-error-handler/hono-error-handler.service.ts (1)
71-92: Consider semantic accuracy for HTTP 502 error code.HTTP 502 is "Bad Gateway", not "Service Unavailable" (which is 503). Mapping both to
"service_unavailable"may cause confusion when debugging. Consider using"bad_gateway"for 502.♻️ Suggested change for semantic accuracy
case 429: return "rate_limited"; case 502: - return "service_unavailable"; + return "bad_gateway"; case 503: return "service_unavailable";apps/tx-signer/src/services/hono-error-handler/hono-error-handler.service.spec.ts (2)
6-15: Consider adding tests for other error branches.The test only covers the fallback path for unknown errors. The
handlemethod has four distinct branches (HTTPException, HttpError, ZodError, and fallback). Testing only one path leaves significant coverage gaps.🧪 Suggested additional test cases
import { mock } from "jest-mock-extended"; import { HTTPException } from "hono/http-exception"; import createHttpError from "http-errors"; import { ZodError } from "zod"; import type { AppContext } from "../../types/app-context"; import { HonoErrorHandlerService } from "./hono-error-handler.service"; describe(HonoErrorHandlerService.name, () => { describe("handle", () => { it("returns 500 response for unknown errors", async () => { const { service, context } = setup(); const response = await service.handle(new Error("boom"), context); expect(response.status).toBe(500); }); it("returns appropriate status for HTTPException", async () => { const { service, context } = setup(); const response = await service.handle(new HTTPException(404, { message: "Not found" }), context); expect(response.status).toBe(404); }); it("returns 400 for ZodError", async () => { const { service, context } = setup(); const zodError = new ZodError([]); const response = await service.handle(zodError, context); expect(response.status).toBe(400); }); it("returns appropriate status for HttpError", async () => { const { service, context } = setup(); const response = await service.handle(createHttpError(403, "Forbidden"), context); expect(response.status).toBe(403); }); }); function setup() { const service = new HonoErrorHandlerService(); const context = mock<AppContext>({ json: ((body: unknown, init: ResponseInit) => new Response(JSON.stringify(body), init)) as AppContext["json"] }); return { service, context }; } });
6-15: Consider using asetupfunction for consistency.Per coding guidelines, tests should use a
setupfunction at the bottom of the rootdescribeblock. While this single-test file works, adopting the pattern now makes it easier to add tests later. As per coding guidelines: "Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it."apps/tx-signer/src/routes/tx/tx.router.spec.ts (1)
7-7: Use<Subject>.namefor the root describe.Hardcoded suite name makes refactors harder. Consider binding the suite to the subject name (e.g., constructor name) to follow the test conventions.
🔧 Example adjustment
-describe("txRouter", () => { +describe(txRouter.constructor.name, () => {As per coding guidelines: Use
<Subject>.namein the root describe suite description instead of hardcoded class/service name strings to enable automated refactoring tools to find all references.
apps/api/src/billing/services/external-signer-http-sdk/external-signer-http-sdk.service.ts
Show resolved
Hide resolved
apps/tx-signer/src/services/hono-error-handler/hono-error-handler.service.ts
Show resolved
Hide resolved
ygrishajev
force-pushed
the
feature/tx-signer
branch
from
eb3fff9 to
1c5e84f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/api/env/.env.functional.test`:
- Around line 37-45: Reorder the environment keys in
apps/api/env/.env.functional.test to satisfy dotenv-linter (alphabetical/order
rules) — specifically ensure FEATURE_FLAGS_ENABLE_ALL,
FEATURE_FLAGS_EXTERNAL_TX_SIGNER_DISABLED, CONSOLE_WEB_PAYMENT_LINK,
STD_OUT_LOG_FORMAT and the FUNDING_/DERIVATION_WALLET_MNEMONIC_V1/V2 entries
follow the linter's expected ordering and grouping, and add a single trailing
newline at the end of the file to fix the EOF newline warning; update the file
so the mnemonic lines remain together and in the correct order per the linter.
- Around line 42-45: The committed plaintext wallet mnemonics
(FUNDING_WALLET_MNEMONIC_V1, DERIVATION_WALLET_MNEMONIC_V1,
FUNDING_WALLET_MNEMONIC_V2, DERIVATION_WALLET_MNEMONIC_V2) must be replaced with
non-sensitive placeholders in apps/api/env/.env.functional.test and real values
should be loaded from CI/local secrets or an example file; change each value to
a clear placeholder (e.g. "__PLACEHOLDER__" or unset) and add instructions to
load the actual mnemonics from environment secrets or a
.env.functional.test.example, ensuring no real mnemonics remain in the repo.
In `@apps/tx-signer/src/config/env.config.ts`:
- Around line 4-7: The schema currently marks FUNDING_WALLET_MNEMONIC_V1,
FUNDING_WALLET_MNEMONIC_V2, DERIVATION_WALLET_MNEMONIC_V1, and
DERIVATION_WALLET_MNEMONIC_V2 as optional, which lets the Wallet constructor
silently generate ephemeral mnemonics; update validation so at least one of
these env vars is present and fail early: add a runtime check in the
WALLET_RESOURCES initialization (or modify the zod schema) that inspects
FUNDING_WALLET_MNEMONIC_V1|V2 and DERIVATION_WALLET_MNEMONIC_V1|V2 and throws a
clear error if all four are undefined, referencing the environment symbol names
in the error message so startup fails loudly instead of creating ephemeral
wallets.
🧹 Nitpick comments (2)
apps/tx-signer/src/config/env.config.ts (1)
3-16: Schema structure looks good and follows existing patterns.The configuration aligns well with the patterns established in
apps/api/src/billing/config/env.config.tsandapps/api/src/core/config/env.config.ts. The use of coercion for numeric fields is correct since environment variables are strings.One minor observation:
RPC_NODE_ENDPOINTcould benefit from.url()validation for stronger input checking, similar to howREST_API_NODE_URLis validated in the core config.💡 Optional: Add URL validation
- RPC_NODE_ENDPOINT: z.string(), + RPC_NODE_ENDPOINT: z.string().url(),apps/tx-signer/src/services/tx-manager/tx-manager.service.ts (1)
99-102: Consider accepting WalletOptions for funding address lookups.
This keeps read-only address retrieval consistent with the v1/v2 support already exposed for derived wallets.Based on learnings: In the Akash Console codebase, v1 wallets are only used for read-only purposes such as retrieving addresses.♻️ Suggested change
- async getFundingWalletAddress() { - const { masterWallet } = this.#getWalletResources(); + async getFundingWalletAddress(options?: WalletOptions) { + const { masterWallet } = this.#getWalletResources(options); return await masterWallet.getFirstAddress(); }
ygrishajev
force-pushed
the
feature/tx-signer
branch
from
1c5e84f to
cfc119b
Compare
ygrishajev
force-pushed
the
feature/tx-signer
branch
4 times, most recently
from
97250d7 to
1433e56
Compare
ygrishajev
force-pushed
the
feature/tx-signer
branch
from
3df22b4 to
d7f85b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/tx-signer/src/services/shutdown-server/shutdown-server.ts`:
- Around line 11-17: The current Promise.resolve(onShutdown?.()) calls
onShutdown synchronously so thrown errors escape the .catch; change it to defer
invocation by using Promise.resolve().then(() => onShutdown?.()) so any
synchronous throw is converted into a rejection and will be handled by the
existing .catch (which logs via appLogger.error with event "ON_SHUTDOWN_ERROR")
before the .finally which calls resolve().
🧹 Nitpick comments (9)
apps/tx-signer/test/seeders/akash-address.seeder.ts (1)
4-12: LGTM! Optional: more idiomatic array initialization.The implementation correctly generates valid Akash addresses. For a slightly more functional approach, you could use
Uint8Array.from():♻️ Optional refactor
export const createAkashAddress = (): string => { - const addressData = new Uint8Array(20); - - for (let i = 0; i < 20; i++) { - addressData[i] = faker.number.int({ min: 0, max: 255 }); - } + const addressData = Uint8Array.from({ length: 20 }, () => faker.number.int({ min: 0, max: 255 })); return toBech32("akash", addressData); };apps/api/src/billing/services/tx-manager/tx-manager.service.spec.ts (1)
294-314: Verify feature flag name is asserted in tests.The mock for
featureFlagsService.isEnabledreturns a boolean unconditionally. Consider asserting thatisEnabledis called with the specific feature flag name (e.g.,"EXTERNAL_TX_SIGNER") to ensure the correct flag is being checked.💡 Optional enhancement to verify flag name
- const featureFlagsService = mock<FeatureFlagsService>({ - isEnabled: jest.fn().mockReturnValue(input?.featureFlagsEnabled ?? false) - }); + const featureFlagsService = mock<FeatureFlagsService>({ + isEnabled: jest.fn().mockImplementation((flag: string) => { + if (flag === "EXTERNAL_TX_SIGNER") { + return input?.featureFlagsEnabled ?? false; + } + return false; + }) + });Then in the tests, add an assertion:
expect(featureFlagsService.isEnabled).toHaveBeenCalledWith("EXTERNAL_TX_SIGNER");apps/tx-signer/src/services/config/config.service.spec.ts (1)
5-11: Consider adding edge case tests.The test validates the happy path. Consider adding tests for:
- Accessing multiple config values
- Type inference with different schema shapes
This would improve confidence in the generic type handling.
apps/tx-signer/src/http-schemas/tx-signer/tx-signer.schema.spec.ts (2)
3-3: Use schema reference in describe block.Per coding guidelines, use a reference instead of hardcoded strings in describe blocks to enable automated refactoring tools.
♻️ Suggested fix
-describe("tx-signer schema", () => { +describe(SignAndBroadcastFundingRequestInputSchema.description ?? "SignAndBroadcastFundingRequestInputSchema", () => {Or simply:
-describe("tx-signer schema", () => { +describe("SignAndBroadcastFundingRequestInputSchema", () => {As per coding guidelines: Use
<Subject>.namein the root describe suite description instead of hardcoded class/service name strings.
4-17: Consider adding validation failure tests.The test only covers the happy path. Consider adding tests for invalid inputs to ensure the schema correctly rejects malformed payloads:
- Empty messages array (schema has
.min(1))- Missing
typeUrl- Missing
value💡 Example additional tests
it("rejects empty messages array", () => { const result = SignAndBroadcastFundingRequestInputSchema.safeParse({ data: { messages: [] } }); expect(result.success).toBe(false); }); it("rejects message without typeUrl", () => { const result = SignAndBroadcastFundingRequestInputSchema.safeParse({ data: { messages: [{ value: Buffer.from([1, 2, 3]).toString("base64") }] } }); expect(result.success).toBe(false); });apps/tx-signer/src/controllers/tx/tx.controller.spec.ts (1)
7-33: Align this test with the requiredsetuphelper pattern.
The test constructs its dependencies inline; please move that into asetupfunction at the bottom of the rootdescribeto match the project’s test conventions.♻️ Suggested refactor
describe(TxController.name, () => { it("decodes messages and signs with funding wallet", async () => { - const decodedMessage = { typeUrl: "/test.MsgTest", value: { foo: "bar" } }; - const registry = mock<Registry>({ - decode: jest.fn().mockReturnValue(decodedMessage.value) - }); - const txManagerService = mock<TxManagerService>({ - signAndBroadcastWithFundingWallet: jest.fn().mockResolvedValue({ code: 0, hash: "tx", rawLog: "" }) - }); - - const controller = new TxController(registry, txManagerService); + const { controller, registry, txManagerService, decodedMessage } = setup({ + decodedMessage: { typeUrl: "/test.MsgTest", value: { foo: "bar" } } + }); const result = await controller.signWithFundingWallet({ data: { messages: [ { typeUrl: decodedMessage.typeUrl, value: Buffer.from([1, 2, 3]).toString("base64") } ] } }); expect(registry.decode).toHaveBeenCalledWith({ typeUrl: decodedMessage.typeUrl, value: expect.any(Uint8Array) }); expect(txManagerService.signAndBroadcastWithFundingWallet).toHaveBeenCalledWith([decodedMessage]); expect(result.data.code).toBe(0); }); + + const setup = ({ + decodedMessage + }: { + decodedMessage: { typeUrl: string; value: { foo: string } }; + }) => { + const registry = mock<Registry>(); + registry.decode.mockReturnValue(decodedMessage.value); + const txManagerService = mock<TxManagerService>(); + txManagerService.signAndBroadcastWithFundingWallet.mockResolvedValue({ code: 0, hash: "tx", rawLog: "" }); + const controller = new TxController(registry, txManagerService); + return { controller, registry, txManagerService, decodedMessage }; + }; });As per coding guidelines: Use
setupfunction instead ofbeforeEachin test files. Thesetupfunction must be at the bottom of the rootdescribeblock, should create an object under test and return it, accept a single parameter with inline type definition, avoid shared state, and not have a specified return type.apps/tx-signer/src/lib/batch-signing-client/batch-signing-client.service.spec.ts (1)
24-36: Test descriptions should not use "should" prefix.Per coding guidelines, test descriptions should use present simple, 3rd person singular without prepending "should". This applies to all test cases in this file.
♻️ Suggested test description changes
- it("should batch and execute multiple transactions successfully", async () => { + it("batches and executes multiple transactions successfully", async () => {Similar changes for other tests:
"handles errors in batch without affecting other transactions""retries failed transaction within a batch on sequence mismatch error and eventually succeeds""recovers transaction when getTx fails with network error but tx exists on chain""recovers transaction when getTx fails with socket error""recovers transaction when getTx fails with cosmjs fetch failed error with cause""does not attempt recovery for non-network errors"apps/tx-signer/src/lib/batch-signing-client/batch-signing-client.service.ts (1)
49-52: Type narrowing for error in retry handler.The retry handler accesses
res.val.messagewithout type narrowing. While this works at runtime, it relies on implicit type coercion.♻️ Consider explicit type check
private readonly signAndBroadcastExecutor = retry( - handleWhenResult(res => res instanceof Err && "message" in res.val && res.val.message?.includes("account sequence mismatch")), + handleWhenResult(res => res instanceof Err && res.val instanceof Error && res.val.message?.includes("account sequence mismatch")), { maxAttempts: 5, backoff: new ExponentialBackoff({ maxDelay: 5_000, initialDelay: 500 }) } );apps/tx-signer/src/controllers/tx/tx.controller.ts (1)
45-55: Consider error handling for message decoding.The
decodeMessagesmethod will throw if:
message.valuecontains invalid base64registry.decodefails for an unknown typeUrlWhile these errors will propagate to the caller, consider whether specific error handling or logging would be beneficial for debugging malformed requests.
ygrishajev
force-pushed
the
feature/tx-signer
branch
from
cd7eb20 to
8966986
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/api/src/billing/services/external-signer-http-sdk/external-signer-http-sdk.service.ts`:
- Around line 71-74: The URL is constructed outside the try in the post method
which can throw (new URL(path, baseUrl)) and bypass LoggerService error
handling; move the URL construction (the call to
billingConfigService.get("TX_SIGNER_BASE_URL") and the new URL(path,
baseUrl).toString() usage) inside the try block in the post<T>(path: string,
body: unknown) method so any exceptions from invalid base URLs or path are
caught and logged by LoggerService within the existing try/catch.
🧹 Nitpick comments (3)
apps/tx-signer/src/http-schemas/tx.schema.ts (2)
14-26: derivationIndex constraint is correct; consider validating granter is non-empty.The
derivationIndexnow properly enforces non-negative integers. One minor consideration:grantercurrently accepts empty strings, which would be invalid as a Cosmos address.💡 Optional: Prevent empty granter string
fee: z.object({ - granter: z.string() + granter: z.string().min(1) })
28-36: Response schema looks good;codecould optionally be constrained to integer.The
.passthrough()is appropriate for allowing additional response fields. Transaction response codes are always integers (0 for success, non-zero for errors), so adding.int()would make the schema more precise.💡 Optional: Constrain code to integer
data: z .object({ - code: z.number(), + code: z.number().int(), hash: z.string(), rawLog: z.string() })apps/api/src/billing/services/external-signer-http-sdk/external-signer-http-sdk.service.ts (1)
75-80: Consider making the HTTP timeout configurable (optional).A hardcoded timeout works, but making it configurable via BillingConfigService keeps it aligned with other environments and avoids code changes to tune it.
♻️ Possible refactor
- const response = await axios.post<T>(url, body, { + const timeoutMs = Number(this.billingConfigService.get("TX_SIGNER_HTTP_TIMEOUT_MS") ?? 60_000); + const response = await axios.post<T>(url, body, { headers: { "Content-Type": "application/json" }, - timeout: 60_000 + timeout: Number.isFinite(timeoutMs) ? timeoutMs : 60_000 });If adding a new env/config key is out of scope for this PR, consider tracking it as a follow-up issue. Based on learnings: In the Akash Network Console project, when cross-cutting concerns or broader refactoring issues are identified during PR review, the preferred approach is to create a separate GitHub issue to track the work rather than expanding the scope of the current PR. This maintains focus and allows for proper planning of architectural improvements.
| private async post<T>(path: string, body: unknown): Promise<T> { | ||
| const baseUrl = this.billingConfigService.get("TX_SIGNER_BASE_URL"); | ||
| const url = new URL(path, baseUrl).toString(); | ||
| try { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "external-signer-http-sdk.service.ts" 2>/dev/null | head -5Repository: akash-network/console
Length of output: 47
🏁 Script executed:
fd "external-signer-http-sdk.service.ts" --type fRepository: akash-network/console
Length of output: 155
🏁 Script executed:
cat -n apps/api/src/billing/services/external-signer-http-sdk/external-signer-http-sdk.service.tsRepository: akash-network/console
Length of output: 4329
🏁 Script executed:
rg -n "TX_SIGNER_BASE_URL" -g'*.ts'Repository: akash-network/console
Length of output: 309
🏁 Script executed:
fd "billing-config.service.ts" --type fRepository: akash-network/console
Length of output: 135
🏁 Script executed:
cat -n apps/api/src/billing/services/billing-config/billing-config.service.tsRepository: akash-network/console
Length of output: 787
🏁 Script executed:
cat -n apps/api/src/billing/config/env.config.tsRepository: akash-network/console
Length of output: 2282
Move URL construction into the try block to ensure errors are logged consistently.
new URL(path, baseUrl) on line 73 can throw for invalid URL formats before reaching the try block, bypassing your standardized LoggerService error handling. Although TX_SIGNER_BASE_URL is validated as a non-empty string at startup (env.config.ts line 40), string validation does not guarantee valid URL format. Move the URL construction into the try block so all failures are logged via LoggerService.
🛠️ Suggested minimal fix
private async post<T>(path: string, body: unknown): Promise<T> {
const baseUrl = this.billingConfigService.get("TX_SIGNER_BASE_URL");
+ try {
- const url = new URL(path, baseUrl).toString();
- try {
+ const url = new URL(path, baseUrl).toString();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async post<T>(path: string, body: unknown): Promise<T> { | |
| const baseUrl = this.billingConfigService.get("TX_SIGNER_BASE_URL"); | |
| const url = new URL(path, baseUrl).toString(); | |
| try { | |
| private async post<T>(path: string, body: unknown): Promise<T> { | |
| const baseUrl = this.billingConfigService.get("TX_SIGNER_BASE_URL"); | |
| try { | |
| const url = new URL(path, baseUrl).toString(); |
🤖 Prompt for AI Agents
In
`@apps/api/src/billing/services/external-signer-http-sdk/external-signer-http-sdk.service.ts`
around lines 71 - 74, The URL is constructed outside the try in the post method
which can throw (new URL(path, baseUrl)) and bypass LoggerService error
handling; move the URL construction (the call to
billingConfigService.get("TX_SIGNER_BASE_URL") and the new URL(path,
baseUrl).toString() usage) inside the try block in the post<T>(path: string,
body: unknown) method so any exceptions from invalid base URLs or path are
caught and logged by LoggerService within the existing try/catch.
8966986 to
8e690fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/deploy-web/src/queries/useTemplateQuery.tsx`:
- Around line 155-161: The query currently sets staleTime and gcTime to
Number.POSITIVE_INFINITY and disables all refetch triggers, so clients never see
backend template updates; change the cache policy in useTemplateQuery.tsx by
replacing Number.POSITIVE_INFINITY for staleTime/gcTime with a bounded duration
(e.g., 5–15 minutes) or enable refetch triggers (set refetchOnWindowFocus: true
and/or refetchOnReconnect: true), and/or ensure callers invalidate
QueryKeys.getTemplatesKey() when templates are updated (e.g., after manual
refresh action); update the options spread around staleTime/gcTime/refetch* in
the query config so QueryKeys.getTemplatesKey()/useTemplateQuery will receive
fresh data.
8e690fa to
5acf322
Compare
3 participants
Summary by CodeRabbit
New Features
Infrastructure
Tests
✏️ Tip: You can customize this high-level summary in your review settings.