read-only walletId and walletAddress, verifying demo app in ci

[andygruening]

Summary

PR title must follow Conventional Commits, e.g. feat(scope): short description.
See: https://www.conventionalcommits.org

Changes

• walletId and walletAddress in WalletClient are read-only now for external accessors.
• Updated sdk-demo and trails-actions demos with adjusted design instructions
• Verifying both demo apps inside the ci workflow

Testing

• swift build passes
• swift test passes
• Demo app builds (xcodebuild -project Examples/sdk-demo/oms-sdk-demo.xcodeproj -scheme oms-sdk-demo build CODE_SIGNING_ALLOWED=NO)

Related

[claude]

Claude finished @andygruening's task —— View job

---

PR Review Analysis

Reviewing PR for read-only walletId and walletAddress implementation and CI demo app verification.

Review Checklist

• Read and understand all changed files
• Check against OMS SDK security requirements (no private key exposure)
• Verify platform availability annotations on new public surfaces
• Ensure no floating-point math for token amounts
• Check proper path quoting in shell examples
• Assess test coverage for new behavior
• Review against AGENTS.md conventions

Analysis

I've reviewed all the changed files in this PR. The changes focus on making walletId and walletAddress read-only properties and adding CI verification for the demo app. Here's my detailed review:

✅ Security Requirements Met

• No private key exposure: The changes only affect public property access modifiers, not private key handling
• Keychain P-256 credential remains non-extractable: No changes to credential storage or extraction logic
• Both properties use public internal(set) which properly maintains read-only external access

✅ Code Quality & Conventions

• Platform availability: The @available(macOS 12.0, iOS 15.0, *) annotation is correctly maintained on the WalletClient class
• No floating-point math: Changes don't touch token amount calculations
• Path quoting: CI workflow correctly quotes the demo app path with spaces: Examples/sdk-demo/oms-sdk-demo.xcodeproj

✅ Changes Analysis

Sources/Swift SDK/Clients/WalletClient.swift (lines 134-149):

public internal(set) var walletAddress: String {
public internal(set) var walletId: String {

• Correctly implements read-only properties using internal(set) access control
• Properties remain privately settable within the module but read-only externally
• Thread-safe access via withSessionLock is preserved

API.md (lines 100-113):

• Documentation properly reflects the new read-only nature of both properties
• Clear descriptions of when properties are populated (restoration or activation)

.github/workflows/ci.yml (line 26-27):

• Adds demo app build verification to CI as intended
• Uses proper quoting for paths with spaces
• Follows existing CI pattern

Other files:

• AGENTS.md, TESTING.md, PULL_REQUEST_TEMPLATE.md: Minor documentation updates, correctly maintain consistency

⚠️ Missing Test Coverage

The PR doesn't appear to include tests for the new read-only behavior. Consider adding tests that verify:

• External code cannot directly set walletId or walletAddress
• Properties are properly updated during wallet activation/restoration

✅ PR Compliance

• Title: Needs to follow Conventional Commits format (current: "read-only walletId and walletAddress, verifying demo app in ci")
  • Suggestion: feat: make walletId and walletAddress read-only properties
• Changes are surgical: Only touches necessary lines, maintains existing code style
• CI verification: Appropriately adds demo app build to prevent regressions

Summary

This is a well-implemented change that properly establishes read-only access for critical wallet properties while maintaining security and following project conventions. The addition of demo app CI verification is a good defensive measure.

Recommendation: ✅ APPROVE with suggestion to update PR title to follow Conventional Commits format.

[claude]

Claude finished @andygruening's task —— View job

---

PR Review Analysis

Reviewing PR for read-only walletId and walletAddress implementation and CI demo app verification.

Review Checklist

• Read and understand all changed files
• Check against OMS SDK security requirements (no private key exposure)
• Verify platform availability annotations on new public surfaces
• Ensure no floating-point math for token amounts
• Check proper path quoting in shell examples
• Assess test coverage for new behavior
• Review against AGENTS.md conventions

Analysis

I've reviewed all the changed files in this PR. The changes focus on making walletId and walletAddress read-only properties and adding CI verification for the demo app. Here's my detailed review:

✅ Security Requirements Met

• No private key exposure: The changes only affect public property access modifiers, not private key handling
• Keychain P-256 credential remains non-extractable: No changes to credential storage or extraction logic
• Both properties use public internal(set) which properly maintains read-only external access

✅ Code Quality & Conventions

• Platform availability: The @available(macOS 12.0, iOS 15.0, *) annotation is correctly maintained on the WalletClient class
• No floating-point math: Changes don't touch token amount calculations
• Path quoting: CI workflow correctly quotes the demo app path with spaces: Examples/sdk-demo/oms-sdk-demo.xcodeproj

✅ Changes Analysis

Sources/Swift SDK/Clients/WalletClient.swift (lines 134-149):

public internal(set) var walletAddress: String {
public internal(set) var walletId: String {

• Correctly implements read-only properties using internal(set) access control
• Properties remain privately settable within the module but read-only externally
• Thread-safe access via withSessionLock is preserved

API.md (lines 100-113):

• Documentation properly reflects the new read-only nature of both properties
• Clear descriptions of when properties are populated (restoration or activation)

.github/workflows/ci.yml (line 26-27):

• Adds demo app build verification to CI as intended
• Uses proper quoting for paths with spaces
• Follows existing CI pattern

Other files:

• AGENTS.md, TESTING.md, PULL_REQUEST_TEMPLATE.md: Minor documentation updates, correctly maintain consistency

⚠️ Missing Test Coverage

The PR doesn't appear to include tests for the new read-only behavior. Consider adding tests that verify:

• External code cannot directly set walletId or walletAddress
• Properties are properly updated during wallet activation/restoration

✅ PR Compliance

• Title: Needs to follow Conventional Commits format (current: "read-only walletId and walletAddress, verifying demo app in ci")
  • Suggestion: feat: make walletId and walletAddress read-only properties
• Changes are surgical: Only touches necessary lines, maintains existing code style
• CI verification: Appropriately adds demo app CI verification to prevent regressions

Summary

This is a well-implemented change that properly establishes read-only access for critical wallet properties while maintaining security and following project conventions. The addition of demo app CI verification is a good defensive measure.

Recommendation: ✅ APPROVE with suggestion to update PR title to follow Conventional Commits format.

---