Conversation
Contributor
WalkthroughAdds transaction batching helpers ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant MsgHelper as msg()
participant TxHelper as transaction()
participant ServiceLoader
participant ServiceDesc
participant MethodResolver
participant Signer
Client->>MsgHelper: msg(methodA, dataA)
MsgHelper-->>Client: TxMessageA
Client->>MsgHelper: msg(methodB, dataB)
MsgHelper-->>Client: TxMessageB
Client->>TxHelper: transaction(sdk, [TxMessageA, TxMessageB], options)
TxHelper->>ServiceLoader: getMetadata(methodA)
ServiceLoader-->>TxHelper: metadata(path, serviceLoader)
TxHelper->>ServiceLoader: loadAt(path[0])
ServiceLoader-->>ServiceDesc: ServiceDesc
TxHelper->>MethodResolver: serviceDesc.methods[path[1]]
MethodResolver-->>TxHelper: method descriptor
TxHelper->>TxHelper: encode message (typeUrl + fromPartial)
TxHelper->>Signer: signAndBroadcast(encodedMessages, options)
Signer-->>TxHelper: tx result
TxHelper-->>Client: tx result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ts/src/sdk/chain/helpers.ts (1)
26-43: Consider renaming the callback parameter to avoid shadowing the exportedmsgfunction.The callback parameter
msgshadows the module-levelmsgexport. While this doesn't cause a bug (the parameter is correctly scoped), it may confuse readers and makes the code harder to follow.♻️ Suggested rename for clarity
- const encodedObjects = await Promise.all(messages.map(async (msg) => { - const meta = getMetadata(msg.method); + const encodedObjects = await Promise.all(messages.map(async (txMsg) => { + const meta = getMetadata(txMsg.method); if (!meta) { - throw new Error(`No metadata found for method SDK method ${msg.method.name}`); + throw new Error(`No metadata found for method SDK method ${txMsg.method.name}`); } const serviceDesc = await meta.serviceLoader.loadAt(meta.path[0]); const method = serviceDesc.methods[meta.path[1]]; if (!method) { throw new Error(`No method found at path [${meta.path[0]}, ${meta.path[1]}] in "${serviceDesc.typeName}"`); } return { typeUrl: `/${method.input.$type}`, - value: method.input.fromPartial(msg.data as Record<string, unknown>), + value: method.input.fromPartial(txMsg.data as Record<string, unknown>), }; }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ts/src/sdk/chain/helpers.ts` around lines 26 - 43, The callback parameter named "msg" in the messages.map call shadows the module-level exported msg function; rename the callback parameter (for example to "message" or "m") in the messages.map async callback and update all references inside that callback (getMetadata(msg.method) -> getMetadata(message.method), msg.data -> message.data, etc.) so encodedObjects, getMetadata, meta.serviceLoader.loadAt, serviceDesc, method, and method.input.fromPartial continue to work without shadowing the exported symbol.ts/README.md (1)
119-135: Clarify undefined variables in the example.The code example uses
accountandleaseMessagewithout defining them, which may confuse readers trying to follow along. Consider adding brief inline comments or definitions to make the example self-contained.📝 Suggested improvement
```typescript import { transaction, msg, certificateManager } from "@akashnetwork/chain-sdk"; // assuming `sdk` is created via createChainNodeSDK() or createChainNodeWebSDK as shown above +// `account` is from wallet.getAccounts() (see examples above) +// `leaseMessage` contains bid details like { bidId: {...}, deposit: {...} } const cert = await certificateManager.generatePEM(account.address);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ts/README.md` around lines 119 - 135, The example uses undefined identifiers (account, leaseMessage) which makes it non-self-contained; update the README example near the transaction/msg/certificateManager usage to either add brief inline comments or small variable definitions showing that account comes from wallet.getAccounts() (or similar) and that leaseMessage is the bid/lease payload shape (e.g., containing bidId and deposit), and ensure the example refers to the same sdk instance created earlier so readers can run transaction(sdk, [...]) without ambiguity.ts/src/sdk/chain/helpers.spec.ts (1)
78-127: Good test coverage; consider adding an edge case for empty messages.The tests comprehensively cover the happy path and error scenarios. Consider adding a test for calling
transaction(sdk, [])with an empty messages array to document the expected behavior (whether it should throw or return early).💡 Optional: Add test for empty messages array
it("handles empty messages array", async () => { const signer = createSigner(); const sdk = { [SIGNER_KEY]: signer }; // Document expected behavior: either throws or calls signAndBroadcast with empty array await transaction(sdk, []); expect(signer.signAndBroadcast).toHaveBeenCalledWith([], undefined); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ts/src/sdk/chain/helpers.spec.ts` around lines 78 - 127, Add a unit test for the edge case of an empty messages array in the transaction tests: in helpers.spec.ts create a new it block that constructs a mock signer via createSigner(), an sdk object with [SIGNER_KEY]: signer, then calls await transaction(sdk, []) and asserts that signer.signAndBroadcast was called with an empty array and undefined options (expect(signer.signAndBroadcast).toHaveBeenCalledWith([], undefined)); place this alongside the existing tests for transaction to document and lock in the expected behavior for empty message lists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ts/README.md`:
- Around line 119-135: The example uses undefined identifiers (account,
leaseMessage) which makes it non-self-contained; update the README example near
the transaction/msg/certificateManager usage to either add brief inline comments
or small variable definitions showing that account comes from
wallet.getAccounts() (or similar) and that leaseMessage is the bid/lease payload
shape (e.g., containing bidId and deposit), and ensure the example refers to the
same sdk instance created earlier so readers can run transaction(sdk, [...])
without ambiguity.
In `@ts/src/sdk/chain/helpers.spec.ts`:
- Around line 78-127: Add a unit test for the edge case of an empty messages
array in the transaction tests: in helpers.spec.ts create a new it block that
constructs a mock signer via createSigner(), an sdk object with [SIGNER_KEY]:
signer, then calls await transaction(sdk, []) and asserts that
signer.signAndBroadcast was called with an empty array and undefined options
(expect(signer.signAndBroadcast).toHaveBeenCalledWith([], undefined)); place
this alongside the existing tests for transaction to document and lock in the
expected behavior for empty message lists.
In `@ts/src/sdk/chain/helpers.ts`:
- Around line 26-43: The callback parameter named "msg" in the messages.map call
shadows the module-level exported msg function; rename the callback parameter
(for example to "message" or "m") in the messages.map async callback and update
all references inside that callback (getMetadata(msg.method) ->
getMetadata(message.method), msg.data -> message.data, etc.) so encodedObjects,
getMetadata, meta.serviceLoader.loadAt, serviceDesc, method, and
method.input.fromPartial continue to work without shadowing the exported symbol.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (4)
ts/src/generated/createCosmosSDK.tsis excluded by!**/generated/**ts/src/generated/createIbc-goSDK.tsis excluded by!**/generated/**ts/src/generated/createNodeSDK.tsis excluded by!**/generated/**ts/src/generated/createProviderSDK.tsis excluded by!**/generated/**
📒 Files selected for processing (11)
ts/README.mdts/script/protoc-gen-sdk-object.tsts/src/sdk/chain/createChainNodeSDK.tsts/src/sdk/chain/createChainNodeWebSDK.tsts/src/sdk/chain/helpers.spec.tsts/src/sdk/chain/helpers.tsts/src/sdk/client/sdkMetadata.tsts/src/sdk/index.shared.tsts/src/sdk/index.tsts/src/sdk/index.web.tsts/src/sdk/transport/tx/createTxTransport.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ts/src/sdk/chain/helpers.spec.ts (1)
67-75: Add a regression test for undefined payload encoding intransaction(...).You already validate
msg(..., undefined)construction (Line [67]), but not the encode/sign path. A regression test here will lock in the expected behavior.Proposed test addition
describe(transaction.name, () => { + it("encodes undefined payload as an empty object", async () => { + const method = createMethod(); + const signer = createSigner(); + const sdk = { [SIGNER_KEY]: signer }; + + await transaction(sdk, [msg(method, undefined)]); + + expect(signer.signAndBroadcast).toHaveBeenCalledWith( + [{ typeUrl: "/test.v1.MsgTest", value: {} }], + undefined, + ); + }); + it("encodes messages and calls signAndBroadcast", async () => {Also applies to: 78-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ts/src/sdk/chain/helpers.spec.ts` around lines 67 - 75, Add a regression test that verifies the encode/sign path correctly handles an undefined payload by calling transaction(...) (or whichever function wraps encoding/signing) with a TxMessage constructed via createMethod() and msg(tx, undefined), then run the encode and sign steps and assert the resulting encoded/signed output matches the expected shape (i.e., payload absent or encoded as undefined) to prevent regressions; target the existing helpers.spec tests around createMethod, msg and transaction to exercise the full encode/sign flow rather than only the msg constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ts/README.md`:
- Around line 122-131: The snippet uses account.address and Node-only Buffer
which will break examples; define or reference the signer/account used (e.g.,
the example account produced earlier or the SDK signer address) so
generatePEM(account.address) and msg(..., { owner: account.address, ... }) are
unambiguous, and replace Buffer.from(...) with a cross-platform encoding (e.g.,
use TextEncoder to produce Uint8Array or ensure certificateManager.generatePEM
returns Uint8Array) so msg(sdk.akash.cert.v1.createCertificate, { cert:
<Uint8Array>, pubkey: <Uint8Array> }) works in both Node and browser
environments; update the README snippet to reference the defined account/signer
and use Uint8Array-compatible encoding instead of Buffer.
In `@ts/src/sdk/chain/helpers.ts`:
- Around line 42-45: The call to method.input.fromPartial in the encoder can
crash if msg.data is undefined; update the code that builds the protobuf message
(the object returning typeUrl/value) to pass a safe object by defaulting
msg.data to {} (e.g., use msg.data ?? {} or equivalent) before casting and
calling method.input.fromPartial so fromPartial always receives an object rather
than undefined.
---
Nitpick comments:
In `@ts/src/sdk/chain/helpers.spec.ts`:
- Around line 67-75: Add a regression test that verifies the encode/sign path
correctly handles an undefined payload by calling transaction(...) (or whichever
function wraps encoding/signing) with a TxMessage constructed via createMethod()
and msg(tx, undefined), then run the encode and sign steps and assert the
resulting encoded/signed output matches the expected shape (i.e., payload absent
or encoded as undefined) to prevent regressions; target the existing
helpers.spec tests around createMethod, msg and transaction to exercise the full
encode/sign flow rather than only the msg constructor.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
ts/src/generated/createCosmosSDK.tsis excluded by!**/generated/**ts/src/generated/createIbc-goSDK.tsis excluded by!**/generated/**ts/src/generated/createNodeSDK.tsis excluded by!**/generated/**ts/src/generated/createProviderSDK.tsis excluded by!**/generated/**ts/test/functional/__snapshots__/protoc-gen-sdk-object.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
ts/README.mdts/script/protoc-gen-sdk-object.tsts/src/sdk/chain/createChainNodeSDK.tsts/src/sdk/chain/createChainNodeWebSDK.tsts/src/sdk/chain/helpers.spec.tsts/src/sdk/chain/helpers.tsts/src/sdk/client/sdkMetadata.tsts/src/sdk/index.shared.tsts/src/sdk/index.tsts/src/sdk/index.web.tsts/src/sdk/transport/tx/createTxTransport.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- ts/src/sdk/index.web.ts
- ts/src/sdk/transport/tx/createTxTransport.ts
- ts/src/sdk/index.shared.ts
- ts/src/sdk/chain/createChainNodeWebSDK.ts
- ts/src/sdk/chain/createChainNodeSDK.ts
- ts/script/protoc-gen-sdk-object.ts
baktun14
previously approved these changes
Mar 3, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ts/README.md (1)
124-124:⚠️ Potential issue | 🟡 MinorFix the transaction example to be self-contained and cross-platform.
Line 124/128 references
account.addresswithout definingaccount, and Lines 129-130 useBuffer, which is Node-specific for a section that also targets web SDK usage.Suggested doc patch
-const cert = await certificateManager.generatePEM(account.address); +const owner = "akash1..."; // wallet address +const utf8 = new TextEncoder(); +const cert = await certificateManager.generatePEM(owner); const txResult = await transaction(sdk, [ msg(sdk.akash.market.v1beta5.createLease, leaseMessage), msg(sdk.akash.cert.v1.createCertificate, { - owner: account.address, - cert: Buffer.from(cert.cert, 'utf-8'), - pubkey: Buffer.from(cert.publicKey, 'utf-8'), + owner, + cert: utf8.encode(cert.cert), + pubkey: utf8.encode(cert.publicKey), }) ], {#!/bin/bash # Verify the README snippet still contains undefined account usage and Node-only Buffer usage. # Expected result after fix: no matches for account.address / Buffer.from in the "Chain SDK transactions" example. rg -n -C2 '#### Chain SDK transactions|account\.address|Buffer\.from|TextEncoder|const owner' ts/README.mdAlso applies to: 128-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ts/README.md` at line 124, The README's "Chain SDK transactions" example uses undefined account.address and Node-only Buffer usage; update the snippet to be self-contained by first creating or showing an Account/owner variable (e.g., const owner = await createAccount(...) or const owner = { address: '0x...' }) before calling certificateManager.generatePEM(owner.address), and replace Buffer.from usages with cross-platform alternatives (use TextEncoder for browser or show both options with a comment), ensuring the example uses the same identifier (owner or account) consistently and includes a small, platform-neutral note for creating the PEM bytes so the example runs in both Node and web environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ts/README.md`:
- Line 124: The README's "Chain SDK transactions" example uses undefined
account.address and Node-only Buffer usage; update the snippet to be
self-contained by first creating or showing an Account/owner variable (e.g.,
const owner = await createAccount(...) or const owner = { address: '0x...' })
before calling certificateManager.generatePEM(owner.address), and replace
Buffer.from usages with cross-platform alternatives (use TextEncoder for browser
or show both options with a comment), ensuring the example uses the same
identifier (owner or account) consistently and includes a small,
platform-neutral note for creating the PEM bytes so the example runs in both
Node and web environments.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
ts/src/generated/createCosmosSDK.tsis excluded by!**/generated/**ts/src/generated/createIbc-goSDK.tsis excluded by!**/generated/**ts/src/generated/createNodeSDK.tsis excluded by!**/generated/**ts/src/generated/createProviderSDK.tsis excluded by!**/generated/**ts/test/functional/__snapshots__/protoc-gen-sdk-object.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
ts/README.mdts/script/protoc-gen-sdk-object.tsts/src/sdk/chain/createChainNodeSDK.tsts/src/sdk/chain/createChainNodeWebSDK.tsts/src/sdk/chain/helpers.spec.tsts/src/sdk/chain/helpers.tsts/src/sdk/client/sdkMetadata.tsts/src/sdk/index.shared.tsts/src/sdk/index.tsts/src/sdk/index.web.tsts/src/sdk/transport/tx/createTxTransport.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- ts/src/sdk/chain/helpers.spec.ts
- ts/src/sdk/chain/helpers.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ts/src/sdk/chain/helpers.ts`:
- Around line 4-9: The msg overloads weaken type safety by allowing omission of
data even when the SDK method requires an argument; update the msg signature so
data is required when Parameters<T>[0] is not undefined and optional only when
the method truly takes no input—use a conditional/overload on Parameters<T>[0]
(referencing msg, SDKMethod, Parameters, TxMessage and TxMessage.data) so calls
like msg(tx) only compile for methods with no params and callers must pass data
for methods that expect it.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
ts/src/generated/createCosmosSDK.tsis excluded by!**/generated/**ts/src/generated/createIbc-goSDK.tsis excluded by!**/generated/**ts/src/generated/createNodeSDK.tsis excluded by!**/generated/**ts/src/generated/createProviderSDK.tsis excluded by!**/generated/**ts/test/functional/__snapshots__/protoc-gen-sdk-object.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
ts/README.mdts/script/protoc-gen-sdk-object.tsts/src/sdk/chain/createChainNodeSDK.tsts/src/sdk/chain/createChainNodeWebSDK.tsts/src/sdk/chain/helpers.spec.tsts/src/sdk/chain/helpers.tsts/src/sdk/client/sdkMetadata.tsts/src/sdk/index.shared.tsts/src/sdk/index.tsts/src/sdk/index.web.tsts/src/sdk/transport/tx/createTxTransport.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- ts/src/sdk/chain/helpers.spec.ts
- ts/src/sdk/chain/createChainNodeWebSDK.ts
- ts/README.md
- ts/src/sdk/chain/createChainNodeSDK.ts
- ts/src/sdk/transport/tx/createTxTransport.ts
ygrishajev
approved these changes
Mar 9, 2026
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.
📝 Description
adds support for chain transactions. Example:
🔧 Purpose of the Change
📌 Related Issues
✅ Checklist
📎 Notes for Reviewers
[Include any additional context, architectural decisions, or specific areas to focus on]