Add TSS - GG20 support #1004
Add TSS - GG20 support #1004
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request updates the desktop runtime and introduces several new iOS components. The desktop change modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FFS as FeatureFlagService
participant Server
Client->>FFS: isFeatureEnabled(feature)
FFS->>Server: getFeatureFlagFromServer()
Server-->>FFS: JSON feature flags
FFS->>Client: Return feature flag (Bool)
sequenceDiagram
participant Client
participant TSS as TssService
participant Messenger as TssMessengerImpl
participant Poller as MessagePuller
participant TSSImpl as TssServiceImpl
participant Server
Client->>TSS: KeygenWithRetry(hexChainCode)
TSS->>Messenger: Send encrypted keygen message
Messenger->>Server: POST message
Server-->>Messenger: Acknowledge message
TSS->>Poller: Start polling for messages
Poller->>Server: GET messages
Server-->>Poller: Return messages
Poller->>TSSImpl: Process inbound messages
TSSImpl->>TSS: Return key generation result
TSS->>Client: Deliver generated key
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (13)
clients/mobile/modules/mobile-tss/ios/LocalStateAccessorImpl.swift (3)
9-18: Consider renaming to be more domain-specific.The error type implementation is clean and follows Swift best practices. However, consider renaming it to be more specific to the TSS local state domain (e.g.,
TssLocalStateError) to better indicate its usage context.-struct TssRuntimeError: LocalizedError { +struct TssLocalStateError: LocalizedError {
22-28: Simplify dictionary initialization.The
Dictionary(uniqueKeysWithValues:)initialization is unnecessary since Dictionary already ensures unique keys.init(localStateDict: [String : String]? = nil) { - if let localStateDict = localStateDict { - self.localStateDict = Dictionary(uniqueKeysWithValues: localStateDict.map { ($0.key, $0.value) }) - } else { - self.localStateDict = [:] - } + self.localStateDict = localStateDict ?? [:] }
38-46: Enhance error messages with more context.The error messages could be more specific about the operation context.
func saveLocalState(_ pubkey: String?, localState: String?) throws { guard let pubkey else { - throw TssRuntimeError("pubkey is nil") + throw TssRuntimeError("Cannot save local state: pubkey is nil") } guard let localState else { - throw TssRuntimeError("localstate is nil") + throw TssRuntimeError("Cannot save local state: local state value is nil") } localStateDict[pubkey] = localState }clients/mobile/modules/mobile-tss/ios/Utils.swift (1)
9-9: Add documentation to explain the enum's purpose.Add a documentation comment to explain that this enum serves as a namespace for utility functions.
+/// Utility functions for message processing and cryptographic operations. enum Utils {clients/mobile/modules/mobile-tss/ios/Tss.swift (3)
29-30: Clarify unused properties.
ThetssMessengerandlocalStateAccessorproperties are defined asniland never used. Consider removing them or implementing a default assignment to avoid confusion.- private let tssMessenger: TssMessengerProtocol? = nil - private let localStateAccessor: LocalStateAccessorImpl? = nil
70-71: Return statement does not match the comment's intent.
Although the comment says “// return from here,” the function continues and returns an empty string at line 80 after successful execution. This may obscure intent for future maintainers. Consider returning a meaningful value or removing the comment to avoid confusion....Also applies to: 80-80
84-84: Address the TODO comment.
SwiftLint warns about the TODO at line 84. If you still need to implement the logic to “wait for keygen to start,” consider tracking it in an issue or providing a placeholder implementation.Would you like me to open an issue or propose a placeholder code snippet enforcing a wait mechanism?
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 84-84: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 84-84: TODOs should be resolved (wait for keygen to start)
(todo)
clients/mobile/modules/mobile-tss/ios/MessagePuller.swift (2)
17-17: Remove redundant initialization of optionals.
SwiftLint highlights that initializing an optional variable withnilis redundant since the default is alreadynil.- private var currentTask: Task<Void,Error>? = nil + private var currentTask: Task<Void,Error>? ... - var decryptedBody: String? = nil + var decryptedBody: String?Also applies to: 106-106
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 17-17: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
51-51: Optional: Implement exponential backoff.
You’re currently sleeping for exactly one second between polls. For better resilience, consider implementing exponential backoff or a maximum retry count.clients/mobile/modules/mobile-tss/ios/Message.swift (1)
9-14: Consider Swift naming conventions for property consistency.
Properties likesession_idorsequence_nouse snake_case. Swift style typically favors camelCase (e.g.,sessionId,sequenceNo). If you need these names for JSON encoding/decoding, consider using customCodingKeysto preserve external naming while keeping your Swift code idiomatic.Also applies to: 16-28
clients/mobile/modules/mobile-tss/ios/FeatureFlagService.swift (1)
9-17: Consider using raw values directly instead of computed property.The
nameproperty duplicates the string representation that could be derived from the raw value. This could lead to inconsistencies if not maintained properly.Consider this simplified implementation:
enum FeatureFlag: String { - case EncryptGCM - - var name: String{ - switch self { - case .EncryptGCM: return "encrypt-gcm" - } - } + case EncryptGCM = "encrypt-gcm" }clients/mobile/modules/mobile-tss/ios/KeygenVerify.swift (1)
55-83: Improve timeout and backoff configuration.Consider extracting magic numbers and implementing exponential backoff for better retry handling.
+ private struct Constants { + static let timeout: TimeInterval = 60 + static let initialBackoff: TimeInterval = 1 + static let maxBackoff: TimeInterval = 8 + } + func checkCompletedParties() async throws -> Bool { let urlString = "\(serverURL)/complete/\(sessionID)" let start = Date() + var currentBackoff = Constants.initialBackoff + guard let url = URL(string: urlString) else { throw TssRuntimeError("invalid url: \(urlString)") } var request = URLRequest(url: url) request.httpMethod = "GET" request.addValue("application/json", forHTTPHeaderField: "Content-Type") repeat{ do { let (data, _) = try await URLSession.shared.data(for: request) if !data.isEmpty { let decoder = JSONDecoder() let peers = try decoder.decode([String].self, from: data) if Set(self.keygenCommittee).isSubset(of: Set(peers)) { self.logger.info("all parties have completed keygen successfully") return true } } - try await Task.sleep(for: .seconds(1)) // backoff for 1 second + try await Task.sleep(for: .seconds(currentBackoff)) + currentBackoff = min(currentBackoff * 2, Constants.maxBackoff) } catch { self.logger.error("Failed to decode response to JSON: \(error)") } - } while (Date().timeIntervalSince(start) < 60) // set timeout to 1 minutes + } while (Date().timeIntervalSince(start) < Constants.timeout) return false }clients/mobile/modules/mobile-tss/ios/Messenger.swift (1)
11-26: Remove redundant counter initialization.The counter is initialized twice: once at declaration and once in init.
let encryptGCM: Bool - var counter: Int64 = 1 + var counter: Int64 private let logger = Logger(subsystem: "messenger", category: "tss") init(serverURL: String, sessionID: String, messageID: String?, encryptionKeyHex: String, encryptGCM: Bool) { self.serverURL = serverURL self.sessionID = sessionID self.messageID = messageID self.encryptionKeyHex = encryptionKeyHex self.encryptGCM = encryptGCM self.counter = 1 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
clients/desktop/wailsjs/runtime/runtime.d.ts(1 hunks)clients/mobile/modules/mobile-tss/ios/FeatureFlagService.swift(1 hunks)clients/mobile/modules/mobile-tss/ios/KeygenVerify.swift(1 hunks)clients/mobile/modules/mobile-tss/ios/LocalStateAccessorImpl.swift(2 hunks)clients/mobile/modules/mobile-tss/ios/Message.swift(1 hunks)clients/mobile/modules/mobile-tss/ios/MessagePuller.swift(1 hunks)clients/mobile/modules/mobile-tss/ios/Messenger.swift(1 hunks)clients/mobile/modules/mobile-tss/ios/Tss.swift(1 hunks)clients/mobile/modules/mobile-tss/ios/Utils.swift(1 hunks)clients/mobile/modules/mobile-tss/ios/Vault.swift(1 hunks)go.mod(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- clients/mobile/modules/mobile-tss/ios/Vault.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
clients/mobile/modules/mobile-tss/ios/Messenger.swift
[Warning] 54-54: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
clients/mobile/modules/mobile-tss/ios/Tss.swift
[Warning] 84-84: Prefer at least one space after slashes for comments
(comment_spacing)
[Warning] 84-84: TODOs should be resolved (wait for keygen to start)
(todo)
clients/mobile/modules/mobile-tss/ios/MessagePuller.swift
[Warning] 17-17: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 106-106: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (App, windows/amd64, windows-latest)
- GitHub Check: build (App, linux/amd64, ubuntu-22.04)
🔇 Additional comments (4)
clients/desktop/wailsjs/runtime/runtime.d.ts (1)
137-137:⚠️ Potential issue✅ Verification successful
Breaking change:
WindowSetSizeno longer returns a Promise.The function signature has been changed from returning
Promise<Size>tovoid. This change could break existing code that awaits the window size after setting it.Let's verify the impact on existing code:
🏁 Script executed:
#!/bin/bash # Description: Search for code that awaits WindowSetSize calls # Test: Look for .then() or await usage with WindowSetSize rg -A 2 "WindowSetSize\(.*\).*\.(then|catch)" || rg "await.*WindowSetSize\("Length of output: 77
I'll run a further check to search for any calls to
WindowSetSizethat might be relying on its old Promise-based behavior. One additional search can help confirm whether the function is ever used in an asynchronous (e.g., awaited or chained with.then) context.Once this script is executed and the output reviewed, please verify manually that there are indeed no usages expecting a Promise from
WindowSetSize.
Rewritten review comment:
Breaking Change Alert:
WindowSetSizenow returnsvoidinstead of aPromise<Size>.
- The previous function signature returned a Promise, which allowed async code (using
awaitor.then()) to rely on the function’s output.- Initial searches (including checks for
.then,.catch, orawaitusage) did not reveal any references awaiting a Promise fromWindowSetSize.- Action Required: Please double-check any consumer code to ensure that there are no implicit dependencies on the old Promise-based behavior before merging this change.
🏁 Script executed:
[run_scripts] #!/bin/bash # Description: Search for any occurrence of "WindowSetSize(" in the repository along with some context rg -C 3 "WindowSetSize\(" || echo "No usage of WindowSetSize found"Length of output: 222
Breaking Change Confirmed:
WindowSetSizenow returnsvoidinstead of aPromise<Size>.
- Our searches for use cases where this function is awaited or chained with
.then()/.catch()returned no usage instances.- There is no evidence of consumer code expecting a Promise from
WindowSetSizein the repository.- Action: Confirm that no external module or future usage relies on the previous Promise behavior.
go.mod (2)
70-73:✅ Verification successful
Verify security implications of dependency updates.
Multiple security-related packages have been updated:
- golang.org/x/crypto v0.33.0
- golang.org/x/net v0.35.0
- golang.org/x/sys v0.30.0
- golang.org/x/text v0.22.0
Let's check for any security advisories:
🏁 Script executed:
#!/bin/bash # Description: Check for security advisories in updated packages # Check for security advisories in golang.org/x packages for pkg in "crypto" "net" "sys" "text"; do echo "Checking golang.org/x/$pkg..." gh api graphql -f query='{ securityVulnerabilities(first: 5, ecosystem: GO, package: "golang.org/x/'$pkg'") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' doneLength of output: 5388
Security Review Verified: Dependency Updates Are Secure
The updated dependency versions in
go.modhave been verified against known security advisories, and all updated versions exceed the vulnerable ranges:
golang.org/x/crypto v0.33.0
• Vulnerabilities affecting versions below v0.31.0 have been addressed.golang.org/x/net v0.35.0
• Issues such as non-linear parsing and excessive memory growth are fixed in versions below v0.33.0, v0.4.0, and others—v0.35.0 meets or exceeds these patches.golang.org/x/sys v0.30.0
• The advisory requiring a patch at commit0.0.0-20220412211240-33da011f77adappears to be resolved in v0.30.0.golang.org/x/text v0.22.0
• All advisories (fixed in v0.3.7–v0.3.8 and later) are addressed, as v0.22.0 is well beyond those vulnerable ranges.No security vulnerabilities remain in the updated packages, and there are no further changes required.
15-15:❓ Verification inconclusive
Verify Wails v2.10.0 compatibility and changelog.
The update to Wails v2.10.0 might explain the breaking change in
WindowSetSize. Let's verify the changes and any potential migration steps.
🌐 Web query:
What are the breaking changes in Wails v2.10.0 compared to v2.9.2?💡 Result:
The provided search results don't explicitly mention Wails v2.10.x releases or their breaking changes compared to v2.9.x (the latest documented version appears to be v3 alpha). However, based on historical patterns from earlier major version transitions and migration guides:
Key Breaking Changes Between Recent Major Versions
1️⃣ Binding System Overhaul (v3 Alpha)
Though not yet released as stable:
- Renamed
application.Options.Bind→application.Options.Services- Service instances must now be wrapped using
application.NewService()- Binding generator now uses ID-based calls by default instead of name-based[6]
🛠️ Windows WebView Changes (v2.x)
- Removed support for WebView2Loader environment/registry overrides when using native WebView implementation[1][3]
📦 Structural Changes
- Deprecated
RGBAcolor option in favor ofBackgroundColour[4]- Removed standalone
app.Bind()method - now configured viaoptions.App.Bindarray[3]
⚠️ Development Workflow
- Minimum Go version requirement increased (e.g., Go 1.18+ for v2)[4]
- Removed
-noGenflag forwails devcommand (replaced by-skipbindings)[7]For projects upgrading between minor versions like 2.x → 3.x:
1️⃣ Review migration guides (v1→v3 example)
🛠️ Test build process with new service binding requirements
⚠️ Verify platform-specific WebView configurationsNote: Always check official release notes for specific version transitions.
Citations:
- 1: https://wails.io/changelog/
- 2: https://github.com/wailsapp/wails
- 3: https://wails.io/docs/guides/migrating/
- 4: https://wails.io/docs/reference/options/
- 5: https://wails.io/docs/v2.9.0/guides/migrating
- 6: https://www.usmartcloud.com/wailsapp/wails/releases
- 7: https://github.com/wailsapp/wails/releases
- 8: https://deps.dev/go/github.com%2Fwailsapp%2Fwails%2Fv2/v2.0.0-alpha.65.0.20210605043712-c13257b9e9a4/versions
- 9: models.ts no longer generated after update to v2.9.2 wailsapp/wails#3809
- 10: Go 1.21.0 go mod changes break wails run/build wailsapp/wails#2875
Wails v2.10.0 Update – Verify Compatibility for
WindowSetSizeAfter reviewing available information, the web query did not yield any explicit breaking changes directly affecting
WindowSetSizein v2.10.0 compared to v2.9.2. However, given recent modifications in Wails v2 (such as Windows WebView adjustments and other structural updates), it's still possible that the update may indirectly impact window sizing behavior.Action Items:
- Review Official Release Notes: Confirm via the official changelog or release notes if any undocumented changes affect
WindowSetSize.- Test Functionality: Manually validate that the
WindowSetSizefunction behaves as expected under v2.10.0.- Consult Migration Guides: If issues are observed, refer to any available migration guides for updated practices.
clients/mobile/modules/mobile-tss/ios/KeygenVerify.swift (1)
12-24: LGTM! Well-structured class initialization.Good practices observed:
- Proper use of logger
- Immutable properties
- Clear parameter naming
| var encryptedBody: String? = nil | ||
| if self.encryptGCM { | ||
| print("decrypt with AES+GCM") | ||
| encryptedBody = body.aesEncryptGCM(key: self.encryptionKeyHex) | ||
| } else { | ||
| print("decrypt with AES+CBC") | ||
| encryptedBody = body.aesEncrypt(key: self.encryptionKeyHex) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace print statements with logger and improve encryption handling.
The code uses print statements for logging and could handle encryption more elegantly.
- var encryptedBody: String? = nil
+ let encryptedBody: String?
if self.encryptGCM {
- print("decrypt with AES+GCM")
+ logger.debug("Encrypting with AES+GCM")
encryptedBody = body.aesEncryptGCM(key: self.encryptionKeyHex)
} else {
- print("decrypt with AES+CBC")
+ logger.debug("Encrypting with AES+CBC")
encryptedBody = body.aesEncrypt(key: self.encryptionKeyHex)
}📝 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.
| var encryptedBody: String? = nil | |
| if self.encryptGCM { | |
| print("decrypt with AES+GCM") | |
| encryptedBody = body.aesEncryptGCM(key: self.encryptionKeyHex) | |
| } else { | |
| print("decrypt with AES+CBC") | |
| encryptedBody = body.aesEncrypt(key: self.encryptionKeyHex) | |
| } | |
| let encryptedBody: String? | |
| if self.encryptGCM { | |
| logger.debug("Encrypting with AES+GCM") | |
| encryptedBody = body.aesEncryptGCM(key: self.encryptionKeyHex) | |
| } else { | |
| logger.debug("Encrypting with AES+CBC") | |
| encryptedBody = body.aesEncrypt(key: self.encryptionKeyHex) | |
| } |
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 54-54: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
| func sendWithRetry(req: URLRequest, msg: Message, retry: Int) { | ||
| URLSession.shared.dataTask(with: req) { _, resp, err in | ||
| if let err { | ||
| self.logger.error("fail to send message,error:\(err)") | ||
| if retry == 0 { | ||
| return | ||
| } else { | ||
| self.sendWithRetry(req: req, msg: msg, retry: retry - 1) | ||
| } | ||
| } | ||
| guard let resp = resp as? HTTPURLResponse, (200 ... 299).contains(resp.statusCode) else { | ||
| self.logger.error("invalid response code") | ||
| return | ||
| } | ||
| self.logger.debug("send message (\(msg.hash) to (\(msg.to)) successfully") | ||
| }.resume() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Modernize retry mechanism with async/await and improve error handling.
The current implementation uses completion handlers and lacks proper backoff strategy.
- func sendWithRetry(req: URLRequest, msg: Message, retry: Int) {
+ private struct Constants {
+ static let maxRetries = 3
+ static let initialBackoff: TimeInterval = 1
+ static let maxBackoff: TimeInterval = 8
+ }
+
+ func sendWithRetry(req: URLRequest, msg: Message, retry: Int) async throws {
+ var currentBackoff = Constants.initialBackoff
+ var currentRetry = retry
+
+ while currentRetry >= 0 {
+ do {
+ let (_, response) = try await URLSession.shared.data(for: req)
+
+ guard let httpResponse = response as? HTTPURLResponse else {
+ throw TssRuntimeError("Invalid response type")
+ }
+
+ if (200...299).contains(httpResponse.statusCode) {
+ logger.debug("Message (\(msg.hash)) sent to (\(msg.to)) successfully")
+ return
+ }
+
+ throw TssRuntimeError("Invalid response code: \(httpResponse.statusCode)")
+ } catch {
+ logger.error("Failed to send message: \(error.localizedDescription)")
+
+ if currentRetry == 0 {
+ throw error
+ }
+
+ try await Task.sleep(for: .seconds(currentBackoff))
+ currentBackoff = min(currentBackoff * 2, Constants.maxBackoff)
+ currentRetry -= 1
+ }
+ }
+ }
- URLSession.shared.dataTask(with: req) { _, resp, err in
- if let err {
- self.logger.error("fail to send message,error:\(err)")
- if retry == 0 {
- return
- } else {
- self.sendWithRetry(req: req, msg: msg, retry: retry - 1)
- }
- }
- guard let resp = resp as? HTTPURLResponse, (200 ... 299).contains(resp.statusCode) else {
- self.logger.error("invalid response code")
- return
- }
- self.logger.debug("send message (\(msg.hash) to (\(msg.to)) successfully")
- }.resume()📝 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.
| func sendWithRetry(req: URLRequest, msg: Message, retry: Int) { | |
| URLSession.shared.dataTask(with: req) { _, resp, err in | |
| if let err { | |
| self.logger.error("fail to send message,error:\(err)") | |
| if retry == 0 { | |
| return | |
| } else { | |
| self.sendWithRetry(req: req, msg: msg, retry: retry - 1) | |
| } | |
| } | |
| guard let resp = resp as? HTTPURLResponse, (200 ... 299).contains(resp.statusCode) else { | |
| self.logger.error("invalid response code") | |
| return | |
| } | |
| self.logger.debug("send message (\(msg.hash) to (\(msg.to)) successfully") | |
| }.resume() | |
| } | |
| private struct Constants { | |
| static let maxRetries = 3 | |
| static let initialBackoff: TimeInterval = 1 | |
| static let maxBackoff: TimeInterval = 8 | |
| } | |
| func sendWithRetry(req: URLRequest, msg: Message, retry: Int) async throws { | |
| var currentBackoff = Constants.initialBackoff | |
| var currentRetry = retry | |
| while currentRetry >= 0 { | |
| do { | |
| let (_, response) = try await URLSession.shared.data(for: req) | |
| guard let httpResponse = response as? HTTPURLResponse else { | |
| throw TssRuntimeError("Invalid response type") | |
| } | |
| if (200...299).contains(httpResponse.statusCode) { | |
| logger.debug("Message (\(msg.hash)) sent to (\(msg.to)) successfully") | |
| return | |
| } | |
| throw TssRuntimeError("Invalid response code: \(httpResponse.statusCode)") | |
| } catch { | |
| logger.error("Failed to send message: \(error.localizedDescription)") | |
| if currentRetry == 0 { | |
| throw error | |
| } | |
| try await Task.sleep(for: .seconds(currentBackoff)) | |
| currentBackoff = min(currentBackoff * 2, Constants.maxBackoff) | |
| currentRetry -= 1 | |
| } | |
| } | |
| } |
Summary by CodeRabbit
New Features
Chores