Skip to content

Conversation

@heyllog
Copy link

@heyllog heyllog commented Dec 23, 2025

Towards #146

@github-actions
Copy link

Skipping AI review because this PR is from a fork. A maintainer can start the review by commenting /review in this PR.

@heyllog heyllog changed the title WalletKit: update docs for version 0.0.4 feat(walletkit): update docs for version 0.0.4 Dec 23, 2025
@novusnota
Copy link
Collaborator

/review

Comment on lines +128 to +140
networks: {
[CHAIN.MAINNET]: { // "-239" - Production network
apiClient: {
url: 'https://toncenter.com',
key: 'your-api-key',
},
},
[CHAIN.TESTNET]: { // "-3" - Testing network
apiClient: {
url: 'https://testnet.toncenter.com',
key: 'your-api-key-testnet',
},
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] API key placeholders do not use required <ANGLE_CASE> format

In the networks configuration example, the mainnet and testnet Toncenter API keys are shown as 'your-api-key' and 'your-api-key-testnet' at lines 131–132 and 137–138. These implicit placeholders do not follow the mandated <ANGLE_CASE> placeholder format and are not defined in the surrounding text, which makes it unclear that the values must be replaced and what they represent. This ambiguity can lead to copy‑paste misuse and violates the [HIGH] placeholder rule for code examples.
Out‑of‑diff evidence: https://github.com/ton-org/docs/blob/main/contribute/style-guide-extended.mdx?plain=1#L559-L571.

Please leave a reaction 👍/👎 to this suggestion to improve future reviews for everyone!

Comment on lines +281 to +292
`TonWalletKit.getWallet()` expects a wallet identifier in the `<chainId>:<address>` format. Build it via `createWalletId()` or rely on `getWalletByAddressAndNetwork()`:

```ts title="TypeScript"
const wallet: IWallet = kit.getWallet(walletAdapter.getAddress());
console.log('TON wallet address:', wallet.getAddress());
import { createWalletId, Network, type Wallet } from '@ton/walletkit';

const walletId = createWalletId(Network.mainnet(), walletAdapter.getAddress());
const wallet: Wallet | undefined = kit.getWallet(walletId);

if (wallet) {
console.log('TON wallet address:', wallet.getAddress());
}
```

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Placeholder format and definition for wallet identifier

The description of TonWalletKit.getWallet() introduces the placeholder format <chainId>:<address> without using the prescribed <ANGLE_CASE> placeholder style or defining the placeholders on first use. As written, <chainId> and <address> look like concrete identifiers rather than template tokens and are not explained, which conflicts with the style rule requiring <ANGLE_CASE> placeholders and explicit definitions to avoid copy/paste confusion. The example code below continues to work, but the prose format and lack of definitions can mislead readers about what values to substitute. Updating the placeholders and defining them inline aligns the docs with the style guide while preserving semantics.

Please leave a reaction 👍/👎 to this suggestion to improve future reviews for everyone!

@github-actions
Copy link

Thanks for the updates to ecosystem/ton-connect/walletkit/web: I have several suggestions to align anchors, placeholders, and wording—please apply the inline suggestions.


Per-comment submission: 3 posted, 1 failed.

Unposted inline comments (raw text):

  • ecosystem/ton-connect/walletkit/web/events.mdx:L29-L31

    [HIGH] Broken anchor to removed apiClient configuration

    The transaction emulation Aside in events.mdx still links to /ecosystem/ton-connect/walletkit/web/init#param-api-client, but the init.mdx page no longer defines a ParamField with path="apiClient" (the configuration was replaced by the networks parameter at https://github.com/ton-org/docs/blob/1680866dd8fc90b3dd5b984ec7b6784a49b39bcf/ecosystem/ton-connect/walletkit/web/init.mdx?plain=1#L118-L143). This leaves a normative link pointing to a missing anchor, which violates the style rule forbidding broken or missing anchors. The issue is introduced by this PR’s change to the initialization/configuration section while leaving the referring link unchanged. The primary fix is to update the link in events.mdx to point to an existing anchor that describes how the API client is selected.

    <Aside>  …(truncated)
    

Comment on lines +310 to 317
`kit.removeWallet()` accepts either a `walletId` or the adapter instance itself. Reuse the same identifier as in `getWallet()`, or pass the adapter you used when adding the wallet:

```ts title="TypeScript"
await kit.removeWallet(wallet.getAddress());
const walletId = createWalletId(Network.mainnet(), walletAdapter.getAddress());
await kit.removeWallet(walletId);
// or
await kit.removeWallet(walletAdapter);
```

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Second-person pronoun in removeWallet description

The updated description for kit.removeWallet() includes the phrase “the adapter you used when adding the wallet,” which directly addresses the reader via the second-person pronoun “you.” The documentation style guide forbids using “you/your” to refer to the reader in normative prose, so this sentence violates that rule even though the surrounding example is otherwise correct. The rest of the section already describes behavior objectively, so a neutral rephrasing can preserve meaning without reader-directed language. Adjusting the sentence to describe the adapter instance in a passive, impersonal way resolves the style violation.

Please leave a reaction 👍/👎 to this suggestion to improve future reviews for everyone!

@novusnota novusnota self-requested a review December 23, 2025 13:20
@novusnota novusnota self-assigned this Jan 5, 2026
@novusnota novusnota added this to the Epic: Jan 5 — 19 milestone Jan 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants