-
Notifications
You must be signed in to change notification settings - Fork 77
Conversation
Skipping AI review because this PR is from a fork. A maintainer can start the review by commenting /review in this PR.
|
No actionable comments were generated in the recent review. 🎉 i️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded TON Connect examples to jetton and NFT transfer docs: React/TypeScript snippets that build TEP-74 / NFT transfer message bodies, base64-BOC encode them, and submit transfer, burn, list-for-sale, and buy transactions via tonConnectUI. New example components exported in each document. Changes
Sequence Diagram(s)sequenceDiagram
participant DApp as DApp (web)
participant TON as TON Connect UI
participant Wallet as User Wallet
participant Chain as TON Blockchain
participant Contract as Token/Sale Contract
DApp->>TON: build transaction (base64 BOC message, validUntil)
TON->>Wallet: request signature & send
Wallet->>TON: signed transaction
TON->>Chain: broadcast transaction
Chain->>Contract: deliver message (transfer / burn / sale)
Contract-->>Chain: state update
Chain-->>DApp: confirm transaction status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
standard/tokens/nft/transfer.mdx (1)
273-275: Avoid floating-point math for TON amounts in examples.Using JS floats for currency (
5.0 + 1.0) is error-prone. Prefer nano amounts (bigint) or string literals before conversion.♻️ Suggested refactor
- const nftPrice = 5.0; // TON - const buyAmount = nftPrice + 1.0; + const nftPrice = toNano('5'); // TON + const buyAmount = nftPrice + toNano('1'); const transaction = { ... messages: [ { address: '<NFT_SALE_CONTRACT>', - amount: toNano(buyAmount).toString(), + amount: buyAmount.toString(), }, ], };Also applies to: 281-282, 301-303, 309-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@standard/tokens/nft/transfer.mdx` around lines 273 - 275, Replace JS floating-point TON literals with explicit nano-TON integer representations or string-to-nano conversions: change nftPrice and buyAmount to use BigInt nanos (e.g., nftPriceNano = BigInt(5) * 10n**9n) or keep human strings and convert via your toNano utility (e.g., toNano("5")). Update every other floating TON literal in this file (the other numeric currency variables used around the transfer flow) to follow the same pattern and ensure downstream calls expect the nano/BigInt or converted value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@standard/tokens/jettons/transfer.mdx`:
- Around line 137-165: The TypeScript examples use an undefined tonConnectUI and
a default import; change the import to the named TonConnectUI and instantiate it
before use (e.g., const tonConnectUI = new TonConnectUI(/* config */)); then
call tonConnectUI.sendTransaction(transaction). Update both blocks that
reference tonConnectUI.sendTransaction to ensure they import { TonConnectUI },
create a tonConnectUI instance (with any required config) and use that instance
when sending the transaction.
In `@standard/tokens/nft/transfer.mdx`:
- Around line 121-147: The snippet imports TonConnectUI but never instantiates
it and also uses a default import instead of the named export; change the import
to a named import (import { TonConnectUI } from '@tonconnect/ui') and create an
instance (e.g., const tonConnectUI = new TonConnectUI({ manifestUrl:
'<YOUR_MANIFEST_URL>' })) before calling tonConnectUI.sendTransaction(...);
apply the same fix to both TypeScript blocks where TonConnectUI and
sendTransaction are used.
---
Nitpick comments:
In `@standard/tokens/nft/transfer.mdx`:
- Around line 273-275: Replace JS floating-point TON literals with explicit
nano-TON integer representations or string-to-nano conversions: change nftPrice
and buyAmount to use BigInt nanos (e.g., nftPriceNano = BigInt(5) * 10n**9n) or
keep human strings and convert via your toNano utility (e.g., toNano("5")).
Update every other floating TON literal in this file (the other numeric currency
variables used around the transfer flow) to follow the same pattern and ensure
downstream calls expect the nano/BigInt or converted value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
i️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2b2690f-07d4-4b1c-aeb8-504a9c1d4b26
📒 Files selected for processing (2)
standard/tokens/jettons/transfer.mdxstandard/tokens/nft/transfer.mdx
@delovoyhomie
delovoyhomie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found two safety-critical issues in the new GetGems section.
One example still hardcodes the old EQAIFun... deployer address from the deprecated cookbook, while the current upstream getgems-io/nft-contracts README explicitly warns not to use that contract for listing NFTs. The buy section also needs an explicit verification step before readers send TON directly to a sale contract.
Because these examples move real funds, I think both points should be fixed before merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the jetton wallet needs message_value > jetton_wallet_gas + forward_ton_amount + receiver_gas, but here forward_ton_amount = message value = 0.05 TON so by the time the chain reaches the receiver's jetton wallet there's nothing left to forward as the notification. typical safe values are total ~0.1 TON with forward ~0.02 TON (matches the low-level example below at lines 352/360). this also applies to the bare TS variant on lines 150/160
correct me if i am wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny: the Aside has no type= so it defaults to a note. that's probably what's intended here, but explicit type makes it consistent with the rest of the file (lines 9, 26, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SendMode is imported but never used in this snippet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense to spell out the unit? 5000 could be basis points (50%), per-mille (5%), or hundredths-of-percent — a reader has to dig into the contract source to find out. a single-line comment is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nftPrice + 0.1 happens in JS floats. 5.0 + 0.1 is fine but the moment nftPrice is, say, 0.2, 0.2 + 0.1 = 0.30000000000000004 and toNano rejects it / rounds. safer to keep everything in nanoTON
and drop the .toString()/toNano(...) around buyAmount below since it's already a bigint. same fix in the TS variant at lines 403-404
Uh oh!
There was an error while loading. Please reload this page.
resolves #2078
Summary by CodeRabbit