fix: publish offchain messages #454
Conversation
WalkthroughThis update modifies the internal implementation of the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SDK
participant Account
participant Client
Caller->>SDK: sendMessage(data, options)
SDK->>Account: signMessage(data)
Account-->>SDK: signature {r, s}
SDK->>Client: publishMessage(data, r (hex), s (hex))
Client-->>SDK: result
SDK-->>Caller: result
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.changeset/hot-icons-clean.md (1)
15-15: Add a more descriptive changelog entry.One-line messages such as “fix: publish offchain messages” give no hint about the root cause or the observable impact. Consider adding a short sentence that explains what broke and how it was fixed. This helps consumers of the package decide whether they need to upgrade.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/hot-icons-clean.md(1 hunks)packages/sdk/src/node/index.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check
- GitHub Check: build
| const sig = await _account?.signMessage(data); | ||
|
|
||
| return ok( | ||
| await client.publishMessage(dataString, [ | ||
| sig.r.toString(), | ||
| sig.s.toString(), | ||
| // @ts-expect-error c | ||
| `0x${sig.r.toString(16)}`, | ||
| // @ts-expect-error c | ||
| `0x${sig.s.toString(16)}`, | ||
| ]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
sendMessage can crash when _account is missing and relies on un-typed signature fields.
Problems
await _account?.signMessage(data)returnsundefinedwhen_accountis not supplied, causingsig.r/sig.sto throw at runtime.options.signeris validated (lines 197–199) but never used, so the earlier guard is meaningless.- Starknet
Account.signMessagereturns{ signature: string[] }, not{ r, s }. Accessingsig.r/sig.s(suppressed with@ts-expect-error) breaks type-safety and may format the data incorrectly. - Re-encoding to hex with
toString(16)will fail if the values are already hex strings.
Minimal, type-safe fix:
- const sig = await _account?.signMessage(data);
+ const signer = _account ?? options.signer;
+ if (!signer) {
+ return err(NO_SIGNER);
+ }
+ const { signature } = await signer.signMessage(data); // signature: [felt1, felt2]- await client.publishMessage(dataString, [
- // @ts-expect-error c
- `0x${sig.r.toString(16)}`,
- // @ts-expect-error c
- `0x${sig.s.toString(16)}`,
- ])
+ const formattedSig = signature.map((felt) =>
+ felt.startsWith("0x") ? felt : `0x${BigInt(felt).toString(16)}`
+ );
+ await client.publishMessage(dataString, formattedSig);This removes the undefined crash path, uses the already-validated signer, preserves typing, and eliminates the @ts-expect-error suppression.
📝 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.
| const sig = await _account?.signMessage(data); | |
| return ok( | |
| await client.publishMessage(dataString, [ | |
| sig.r.toString(), | |
| sig.s.toString(), | |
| // @ts-expect-error c | |
| `0x${sig.r.toString(16)}`, | |
| // @ts-expect-error c | |
| `0x${sig.s.toString(16)}`, | |
| ]) | |
| // Ensure we have a signer (either _account or options.signer) | |
| const signer = _account ?? options.signer; | |
| if (!signer) { | |
| return err(NO_SIGNER); | |
| } | |
| // Starknet returns { signature: string[] } | |
| const { signature } = await signer.signMessage(data); | |
| // Format each felt as hex (add 0x if missing) | |
| const formattedSig = signature.map((felt) => | |
| felt.startsWith("0x") ? felt : `0x${BigInt(felt).toString(16)}` | |
| ); | |
| return ok( | |
| await client.publishMessage(dataString, formattedSig) | |
| ); |
🤖 Prompt for AI Agents
In packages/sdk/src/node/index.ts around lines 206 to 214, fix the sendMessage
function to handle the case when _account is missing by removing the optional
chaining on signMessage and ensuring _account is always defined before calling
signMessage. Replace the usage of sig.r and sig.s with the correct signature
format returned by Starknet Account.signMessage, which is an object containing a
signature array of hex strings. Use the already validated options.signer instead
of _account for signing to maintain type safety and remove the @ts-expect-error
comments. Avoid re-encoding signature parts with toString(16) since they are
already hex strings. This will prevent runtime crashes and maintain proper
typing.
Closes #
Introduced changes
Checklist
Summary by CodeRabbit