Feature/ton 1790 polish sdk#120
Open
nikdim03 wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe PR adds typed return values to TonConnect approval operations (transaction, sign-data, sign-message) by switching from fire-and-forget ChangesTonConnect Typed Approval Responses
New API Members: jettons streaming, backResolveDnsWallet, staking/streaming suspend
TONMnemonic equality and model unit tests
README and Android quick-start documentation updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
TONWalletKit-Android/impl/src/test/java/io/ton/walletkit/engine/operations/TonConnectOperationsTest.kt (1)
265-276: ⚡ Quick winAdd parity tests for typed sign-data/sign-message approvals.
This update asserts typed decoding for
approveTransaction, but the same contract shift happened forapproveSignDataandapproveSignMessage. Please add equivalent response-shape assertions for those two paths to guard againstcallTypeddecoding regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TONWalletKit-Android/impl/src/test/java/io/ton/walletkit/engine/operations/TonConnectOperationsTest.kt` around lines 265 - 276, Add two new test methods in the TonConnectOperationsTest class that mirror the structure and assertions of approveTransaction_returnsApprovalResponse, but for the approveSignData and approveSignMessage methods respectively. Each test should use givenBridgeReturns to mock the bridge response, create the appropriate request event (using createSignDataRequestEvent or createSignMessageRequestEvent), call the corresponding rpcClient method (approveSignData or approveSignMessage), and assert that the response shape is correctly decoded through callTyped by verifying the expected fields are properly populated in the returned response object.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@TONWalletKit-Android/api/src/main/java/io/ton/walletkit/model/ITONWalletAdapter.kt`:
- Around line 80-81: The new interface method supportedFeatures() in
ITONWalletAdapter has a default implementation body, which requires JVM
default-method compatibility configuration. Add the `-Xjvm-default=all` compiler
flag to the compilerOptions.freeCompilerArgs in
TONWalletKit-Android/api/build.gradle.kts, or alternatively annotate the
supportedFeatures() method with `@JvmDefault` to ensure existing custom
implementations of this interface remain binary compatible at runtime.
In
`@TONWalletKit-Android/api/src/main/java/io/ton/walletkit/model/TONMnemonic.kt`:
- Around line 119-125: The TONMnemonic class overrides equals and hashCode based
on mutable state (the words field), which violates the contract for use in
hash-based collections like HashSet or HashMap since the object can be mutated
via the updateWord method. Either remove the equals and hashCode method
overrides entirely to rely on reference equality for this mutable class, or add
a KDoc warning on the TONMnemonic class that clearly documents that instances
must never be used as keys in HashMap/HashSet and should not be mutated after
creation. Choose the approach that best fits your design intent for this class.
---
Nitpick comments:
In
`@TONWalletKit-Android/impl/src/test/java/io/ton/walletkit/engine/operations/TonConnectOperationsTest.kt`:
- Around line 265-276: Add two new test methods in the TonConnectOperationsTest
class that mirror the structure and assertions of
approveTransaction_returnsApprovalResponse, but for the approveSignData and
approveSignMessage methods respectively. Each test should use givenBridgeReturns
to mock the bridge response, create the appropriate request event (using
createSignDataRequestEvent or createSignMessageRequestEvent), call the
corresponding rpcClient method (approveSignData or approveSignMessage), and
assert that the response shape is correctly decoded through callTyped by
verifying the expected fields are properly populated in the returned response
object.
🪄 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: 62651f1a-f17f-4a86-a602-6315650fccc6
📒 Files selected for processing (23)
README.mdTONWalletKit-Android/README.mdTONWalletKit-Android/api/src/main/java/io/ton/walletkit/ITONWallet.ktTONWalletKit-Android/api/src/main/java/io/ton/walletkit/ITONWalletKit.ktTONWalletKit-Android/api/src/main/java/io/ton/walletkit/client/TONAPIClient.ktTONWalletKit-Android/api/src/main/java/io/ton/walletkit/model/ITONWalletAdapter.ktTONWalletKit-Android/api/src/main/java/io/ton/walletkit/model/TONMnemonic.ktTONWalletKit-Android/api/src/main/java/io/ton/walletkit/request/RequestHandler.ktTONWalletKit-Android/api/src/main/java/io/ton/walletkit/request/TONWalletConnectionRequest.ktTONWalletKit-Android/api/src/main/java/io/ton/walletkit/request/TONWalletSignDataRequest.ktTONWalletKit-Android/api/src/main/java/io/ton/walletkit/request/TONWalletSignMessageRequest.ktTONWalletKit-Android/api/src/main/java/io/ton/walletkit/request/TONWalletTransactionRequest.ktTONWalletKit-Android/api/src/main/java/io/ton/walletkit/streaming/ITONStreamingManager.ktTONWalletKit-Android/api/src/main/java/io/ton/walletkit/streaming/ITONStreamingProvider.ktTONWalletKit-Android/api/src/test/java/io/ton/walletkit/model/TONHexTest.ktTONWalletKit-Android/api/src/test/java/io/ton/walletkit/model/TONMnemonicTest.ktTONWalletKit-Android/api/src/test/java/io/ton/walletkit/model/TONTokenAmountTest.ktTONWalletKit-Android/impl/src/main/java/io/ton/walletkit/core/TONWalletKit.ktTONWalletKit-Android/impl/src/main/java/io/ton/walletkit/engine/WalletKitEngine.ktTONWalletKit-Android/impl/src/main/java/io/ton/walletkit/engine/WebViewWalletKitEngine.ktTONWalletKit-Android/impl/src/main/java/io/ton/walletkit/engine/operations/TonConnectOperations.ktTONWalletKit-Android/impl/src/test/java/io/ton/walletkit/engine/operations/GaslessOperationsTest.ktTONWalletKit-Android/impl/src/test/java/io/ton/walletkit/engine/operations/TonConnectOperationsTest.kt
Comment on lines
+80
to
81
| /** The wallet features this adapter supports, or null for SDK defaults. */ | ||
| fun supportedFeatures(): List<TONWalletKitConfiguration.Feature>? = null |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Gradle/Kotlin files =="
fd -H -t f 'build.gradle.kts|build.gradle|gradle.properties|settings.gradle.kts|settings.gradle'
echo
echo "== Kotlin JVM default-method/compiler settings =="
rg -n --iglob '*gradle*' --iglob 'gradle.properties' \
'jvmDefault|-Xjvm-default|freeCompilerArgs|kotlinOptions|compilerOptions|JvmDefault'
echo
echo "== API/binary compatibility tooling (if any) =="
rg -n --iglob '*gradle*' 'binary-compatibility-validator|apiValidation|explicitApi'Repository: ton-org/kit-android
Length of output: 1517
🏁 Script executed:
# Read the full API module build.gradle.kts
cat -n TONWalletKit-Android/api/build.gradle.kts | head -80Repository: ton-org/kit-android
Length of output: 2793
🏁 Script executed:
# Examine the ITONWalletAdapter.kt interface file
cat -n TONWalletKit-Android/api/src/main/java/io/ton/walletkit/model/ITONWalletAdapter.ktRepository: ton-org/kit-android
Length of output: 4012
🏁 Script executed:
# Search for `@JvmDefault` annotations in the codebase
rg -n '`@JvmDefault`' --type kotlinRepository: ton-org/kit-android
Length of output: 45
Add JVM default-method compatibility configuration for the new interface member.
Line 81 adds a new interface method with a default body, but the API module's build.gradle.kts lacks the -Xjvm-default compiler flag. Existing precompiled custom ITONWalletAdapter implementations will face binary compatibility issues at runtime. Either:
- Add
-Xjvm-default=alltocompilerOptions.freeCompilerArgsin TONWalletKit-Android/api/build.gradle.kts, or - Annotate the
supportedFeatures()method with@JvmDefault
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@TONWalletKit-Android/api/src/main/java/io/ton/walletkit/model/ITONWalletAdapter.kt`
around lines 80 - 81, The new interface method supportedFeatures() in
ITONWalletAdapter has a default implementation body, which requires JVM
default-method compatibility configuration. Add the `-Xjvm-default=all` compiler
flag to the compilerOptions.freeCompilerArgs in
TONWalletKit-Android/api/build.gradle.kts, or alternatively annotate the
supportedFeatures() method with `@JvmDefault` to ensure existing custom
implementations of this interface remain binary compatible at runtime.
Comment on lines
+119
to
+125
| override fun equals(other: Any?): Boolean { | ||
| if (this === other) return true | ||
| if (other !is TONMnemonic) return false | ||
| return words == other.words | ||
| } | ||
|
|
||
| override fun hashCode(): Int = words.hashCode() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for TONMnemonic usage in HashSet, HashMap, or as map keys
# Search for HashSet/HashMap usage with TONMnemonic
rg -n --type=kotlin -C3 'HashSet.*TONMnemonic|HashMap.*TONMnemonic|mutableSetOf.*TONMnemonic|mutableMapOf.*TONMnemonic'
# Search for TONMnemonic.updateWord calls to confirm mutability usage
rg -n --type=kotlin -C2 '\.updateWord\('Repository: ton-org/kit-android
Length of output: 1786
🏁 Script executed:
cat -n TONWalletKit-Android/api/src/main/java/io/ton/walletkit/model/TONMnemonic.kt | head -140Repository: ton-org/kit-android
Length of output: 5675
🏁 Script executed:
rg -n "class TONHex" --type=kotlin -A 20Repository: ton-org/kit-android
Length of output: 4579
Overriding equals/hashCode on a mutable class is an anti-pattern.
TONMnemonic is intentionally mutable (via updateWord) for UI input handling, but overrides equals and hashCode based on mutable internal state. If an instance is added to a HashSet or used as a HashMap key and then mutated via updateWord, the collection contracts will be violated, leading to silent bugs where the object can no longer be found or removed.
While no current usage in hash-based collections was found, the codebase exposes this as a public API, so external callers could encounter this issue.
Consider one of the following approaches:
- Make
TONMnemonicimmutable by removingupdateWordand makingwordsimmutable. (Conflicts with documented UI input handling use case.) - Remove the
equals/hashCodeoverrides and rely on reference equality for a mutable object. - Document the constraint with a KDoc warning that instances must not be used in hash-based collections and should not be mutated after creation.
Note: Other model classes like TONHex are immutable data classes, suggesting that value-based equality should align with immutability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TONWalletKit-Android/api/src/main/java/io/ton/walletkit/model/TONMnemonic.kt`
around lines 119 - 125, The TONMnemonic class overrides equals and hashCode
based on mutable state (the words field), which violates the contract for use in
hash-based collections like HashSet or HashMap since the object can be mutated
via the updateWord method. Either remove the equals and hashCode method
overrides entirely to rely on reference equality for this mutable class, or add
a KDoc warning on the TONMnemonic class that clearly documents that instances
must never be used as keys in HashMap/HashSet and should not be mutated after
creation. Choose the approach that best fits your design intent for this class.
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
1 participant
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.
Summary by CodeRabbit
Release Notes
New Features
Documentation
API Improvements
staking(),streaming()) now support async operationsTests
Refactor