feat(signing): put in agreement with styleguide and port code samples to Tolk#2127
feat(signing): put in agreement with styleguide and port code samples to Tolk#2127
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request updates the signing technique documentation to align with styleguides and modernize code examples. Changes include restructuring sections from bullet lists to numbered sequences, translating code examples from FunC syntax to Tolk syntax with updated method calls, normalizing formatting conventions, and expanding explanatory content for clarity. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
contract-dev/techniques/signing.mdx (1)
73-79: LanguageTool: repeated sentence openings.In both Example 1 (Lines 75-79) and Example 2 (Lines 98-101), four consecutive steps start with "Wallet contract …". Consider varying the subject (e.g., "It verifies …", "Then checks seqno …") or merging tightly related steps to improve readability. Low priority.
Also applies to: 93-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract-dev/techniques/signing.mdx` around lines 73 - 79, The steps list repeats the subject "Wallet contract" several times in both Example 1 and Example 2; update the step wording to improve flow by varying sentence openings or merging closely related steps: for instance, change some "Wallet contract verifies the signature." to "It verifies the signature.", merge "Wallet contract checks seqno for replay protection." and "Wallet contract accepts the message and pays gas..." into "It checks seqno for replay protection, accepts the message, and pays gas from the wallet balance.", and/or reorder to combine verification, seqno check and acceptance into one coherent step; apply similar edits to the other example so the repeated "Wallet contract ..." phrasing is reduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contract-dev/techniques/signing.mdx`:
- Line 202: The sentence "This approach is used in preprocessed wallet v2,
highload wallet v3" is missing terminal punctuation; edit the line in
signing.mdx (the sentence text) to add a period (or other appropriate terminal
punctuation) at the end so it reads "This approach is used in preprocessed
wallet v2, highload wallet v3." and matches surrounding documentation style.
- Line 254: The sentence currently implies all TVM versions introduce builder
hashing; update the phrasing around `HASHBU` to align with the earlier "Recent
TVM versions" wording and, if possible, name the specific TVM release that added
`HASHBU` (e.g., "since TVM vX.Y.Z") or otherwise prefix with "Recent TVM
versions" and add a citation/reference; ensure the sentence mentions the
`HASHBU` instruction explicitly and clarifies that it is an optimization that
reduces gas cost by hashing builder data rather than treating it as a slice.
- Line 43: Edit the sentence in signing.mdx that states "consistent signing
time" for Ed25519; remove that phrase and rephrase to say Ed25519 signatures in
TON operate on hashes to ensure fixed-size input and to satisfy the 1016-bit /
ignored-refs limitation of CHKSIGNS (keep the existing CHKSIGNS explanation at
Line 61). Update the line mentioning Ed25519 so it only references fixed-size
input and the CHKSIGNS limitation, not timing implications.
---
Nitpick comments:
In `@contract-dev/techniques/signing.mdx`:
- Around line 73-79: The steps list repeats the subject "Wallet contract"
several times in both Example 1 and Example 2; update the step wording to
improve flow by varying sentence openings or merging closely related steps: for
instance, change some "Wallet contract verifies the signature." to "It verifies
the signature.", merge "Wallet contract checks seqno for replay protection." and
"Wallet contract accepts the message and pays gas..." into "It checks seqno for
replay protection, accepts the message, and pays gas from the wallet balance.",
and/or reorder to combine verification, seqno check and acceptance into one
coherent step; apply similar edits to the other example so the repeated "Wallet
contract ..." phrasing is reduced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dce42ec4-76a4-4105-bbc1-3ff11c4be72d
📒 Files selected for processing (1)
contract-dev/techniques/signing.mdx
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Thanks for the updates to Per-comment submission: 1 posted, 1 failed. Unposted inline comments (raw text):
|
|
/review |
| 1. [`CHKSIGNS`](/tvm/instructions#f911-chksigns) checks signature of data. | ||
|
|
||
| Hash-based verification (`CHKSIGNU`) is preferred because `CHKSIGNS` only processes data from a single cell (up to $127 * 8 = 1016$ bits) and ignores cell references. For messages containing multiple cells or references, hashing the entire structure first is required. | ||
| Hash-based verification (`CHKSIGNU`) is preferred because `CHKSIGNS` only processes data from a single cell with a maximum size of 127 × 8 = 1016 bits and ignores [cell references](/foundations/serialization/library). For messages containing multiple cells or references, hashing the entire structure first is required. |
There was a problem hiding this comment.
This link now points readers to the library-reference-cell page, which is about the exotic 0x02 cell type. Here cell references means ordinary refs on any cell, so the general cells page is the right target.
| Hash-based verification (`CHKSIGNU`) is preferred because `CHKSIGNS` only processes data from a single cell with a maximum size of 127 × 8 = 1016 bits and ignores [cell references](/foundations/serialization/library). For messages containing multiple cells or references, hashing the entire structure first is required. | |
| Hash-based verification (`CHKSIGNU`) is preferred because `CHKSIGNS` only processes data from a single cell with a maximum size of 127 × 8 = 1016 bits and ignores [cell references](/foundations/serialization/cells). For messages containing multiple cells or references, hashing the entire structure first is required. |
| title="Optimization available" | ||
| > | ||
| Recent TVM versions support `builder_hash()` for efficient hashing. Convert the slice to a builder and hash it — this costs less than 100 gas total. See [Optimization: Builder hashing](#optimization-builder-hashing) below for details. | ||
| Recent TVM versions support `builder.hash()` for efficient hashing. [Convert the slice to a builder and hash it](#optimization-builder-hashing) — this costs less than 100 gas total. |
There was a problem hiding this comment.
This still leaves the availability of builder.hash() vague. The optimization depends on HASHBU, and upstream lists HASHBU under TVM Version 12: https://github.com/ton-blockchain/ton/blob/5c0349110bb03dd3a241689f2ab334ae1a554ffb/doc/GlobalVersions.md#L232-L273
| Recent TVM versions support `builder.hash()` for efficient hashing. [Convert the slice to a builder and hash it](#optimization-builder-hashing) — this costs less than 100 gas total. | |
| TVM v12 and later versions support `builder.hash()` for efficient hashing. [Convert the slice to a builder and hash it](#optimization-builder-hashing) — this costs less than 100 gas total. |
| ### Optimization: Builder hashing | ||
|
|
||
| Recent TVM versions introduced efficient builder hashing (`HASHBU` instruction), which makes **signed data as slice** approach much more gas-efficient. | ||
| TVM versions introduce builder hashing through the `HASHBU` instruction. This optimization reduces the gas cost of the signed data as a slice approach. |
There was a problem hiding this comment.
This sentence should name the exact TVM version, especially because #2126 is specifically asking to clarify this. The upstream version history lists HASHBU in TVM Version 12: https://github.com/ton-blockchain/ton/blob/5c0349110bb03dd3a241689f2ab334ae1a554ffb/doc/GlobalVersions.md#L232-L273
| TVM versions introduce builder hashing through the `HASHBU` instruction. This optimization reduces the gas cost of the signed data as a slice approach. | |
| Starting with TVM v12, builder hashing is available through the `HASHBU` instruction. This optimization reduces the gas cost of the "signed data as slice" approach. |
2 participants
closes #2116
Summary by CodeRabbit