Skip to content

Align wallet sessions, Solana, and wallet import - #45

Open
tolgahan-arikan wants to merge 10 commits into
masterfrom
align-smart-sessions-solana-wallet-import
Open

tolgahan-arikan wants to merge 10 commits into
masterfrom
align-smart-sessions-solana-wallet-import

Conversation

@tolgahan-arikan

@tolgahan-arikan tolgahan-arikan commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • refresh the internal WaaS client and align the public SDK with TypeScript PR #85
  • add Ethereum and Solana attested wallet import, including Nitro attestation verification and P-256 HPKE with AES-256-GCM
  • add Solana wallet creation, signing, verification, transfers, and indexer balances
  • add wallet key origin, fee-option selection by index, and owner-side remote-access inspection, approval, listing, usage, and revocation
  • update public API baselines, generated API docs, README examples, error contracts, and focused tests

Wallet import

Applications can import raw EVM or Solana private-key material through the SDK's attested HPKE flow or provide encrypted key material through the advanced recipient-key/import methods. The advanced flow supports server-side custody integrations such as Privy. Mnemonic or HD-wallet inputs must first be derived locally to the intended child private key.

Attestation trust is managed by the SDK for Polygon-managed OMS environments. Development publishable keys accept Nitro's debug-mode zero PCR0 for disposable test wallets. Staging and Production use pinned measurements from the corresponding WaaS releases, so callers no longer construct WalletImportConfiguration or supply PCR0 values.

PCR rotation must be coordinated with SDK releases: publish an SDK accepting the current and replacement measurements before deploying the replacement enclave, then remove the retired measurement in a later release.

Explicitly excluded

  • backend-facing RAC registration, execution, and session-management APIs
  • smart-session example or infrastructure

The generated transport remains internal, so upstream backend operations are not part of the public SDK surface.

Generated source

  • repository: 0xsequence/waas
  • branch: integration/v1.1
  • commit: 817701409d8d980979aa03d273538ee112663eaf
  • schema: v1-26.8.24-262bd7ba
  • schema hash: 688ad6c684fc1ffa8aab10d476c7fb2dd60a5019
  • generator: webrpc-gen v0.37.2 with gen-swift v0.1.4

Published measurement sources:

Breaking change

Access grant models and owner access methods now follow the direct and remote access session API. Wallet import and its temporary caller-supplied PCR configuration have not shipped in a Swift SDK release, so removing WalletImportConfiguration does not break an existing released API. No compatibility shim or release-version bump is included.

Pre-merge requirement

  • Confirm stagingWalletImportPcr0s with the WaaS release/deployment owner and verify it matches the intended Staging enclave. The current value matches the published WaaS v1.0.3 Next measurement and the current Staging status response.
  • Confirm productionWalletImportPcr0s with the WaaS release/deployment owner and verify it matches the intended Production enclave. The current value matches the published WaaS v1.0.0 Production measurement, the deployment KMS allowlist, and the current Production status response.
  • Confirm the SDK-first PCR rotation sequence above is part of the deployment process.

Do not merge until these checks are complete. A wrong pin either blocks wallet import or trusts an unintended enclave image.

Verification

  • scripts/verify.sh
  • 120 Swift tests passed
  • Swift build and public API checks passed
  • CocoaPods lint passed
  • SDK demo and Trails Actions demo builds passed
  • git diff --check passed

Refresh the internal WaaS client and align the owner-facing SDK surface with TypeScript. Add Solana wallet and indexer support plus attested wallet import, while excluding backend RAC operations and example infrastructure.

BREAKING CHANGE: access grant models and owner access methods now follow the direct and remote access session API.
@tolgahan-arikan
tolgahan-arikan requested a review from a team September 4, 2026 14:58
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-07T09:15:42.469019Z 1164465 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Claude finished @tolgahan-arikan's task —— View job


PR Review: Align wallet sessions, Solana, and wallet import

Review todo list:

  • Read AGENTS.md conventions
  • Review new wallet import (Attestation, WalletImport, WalletImports)
  • Security: no private key material in session storage, P-256 non-extractable
  • No floating-point math for token amounts
  • Platform availability annotations (@available) on new public surfaces
  • Test coverage for new behavior
  • Shell examples: paths with spaces quoted
  • Post review

Overall this is a solid, security-conscious PR. The attestation verification (Nitro root pinning, PCR0 trust, request/response binding, freshness window) and the local HPKE sealing before import are well constructed and match the AGENTS.md security posture. Below are findings, ordered by importance.

🐞 Bug — duplicate @available attribute

Sources/OMSWallet/Models/Operations/FeeOptionsSelector.swift:29-30

@available(macOS 12.0, iOS 15.0, *)
@available(macOS 12.0, iOS 15.0, *)
public extension FeeOptionSelection {

The annotation is repeated. Harmless at runtime but should be de-duplicated to one line.

⚠️ Potential correctness — Solana indexer URL derivation

Sources/OMSWallet/OMSWalletEnvironment.swift:16

?? "\(walletApiUrl.trimmingCharacters(in: CharacterSet(charactersIn: "/")))/v1/SolanaIndexerGateway/"

trimmingCharacters(in:) only trims the leading/trailing characters, so a walletApiUrl such as https://api.example.com/ correctly becomes https://api.example.com. However, if walletApiUrl ever contains a trailing path segment already (or no scheme), the concatenation assumptions could break. Worth a quick unit test asserting the derived solanaIndexerGatewayUrl for the standard environment URLs, since there is currently no test covering this default. (Not blocking if the URL shape is guaranteed by parsePublishableKey.)

🔒 Security — looks good, one note

  • importWallet seals plaintext locally and zeroes it via defer { plaintext.resetBytes(...) } (WalletImports.swift:39-40). No plaintext key is written to session storage, and the Keychain P-256 credential remains non-extractable. ✅
  • Note for the .ethereum(String) / .solana(String) variants: plaintext is a copy (Data(trimmed.utf8)); the caller-owned String still holds the key material and cannot be zeroed by the SDK. This is inherent to String and acceptable, but it may be worth a one-line doc note that raw-Data variants (.ethereumBytes / .solanaBytes) are preferable when the caller wants to zero their own buffer.
  • force-unwrap on Data(base64Encoded:)! at WalletImports.swift:142-143 is safe because canonicalBase64(...) already validated the strings — good defensive ordering.

✅ No floating-point on token amounts

FeeOptionSelector.hasEnoughBalance / normalizedUnsignedDecimal (FeeOptionsSelector.swift:77-101) compare unsigned decimals lexically by length then value — no Double. formatTokenAmount routes through formatUnits (WalletOperations.swift:612-616). Consistent with the convention. ✅

✅ Platform availability annotations

New public surfaces carry @available(macOS 12.0, iOS 15.0, *): FeeOptionWithBalance, FeeOptionSelector, ListAccessPages, WalletClient extensions, OMSWallet. WalletImportConfiguration and the Solana indexer model structs are plain Sendable value types with no availability-gated API, which is consistent with existing model files. ✅

🧪 Test coverage — good, with gaps

Strong new coverage in WalletImportTests.swift (PCR0 validation, scalar/length boundaries, HPKE encapsulation shape) and MockWalletTests.swift/IndexerTests.swift.

Consider adding:

  • An attestation-verification failure path test (AttestationVerifier.verify) — e.g. nonce mismatch, untrusted PCR0, stale timestamp, or bad user_data binding. Currently the verifier’s many guards are untested; a couple of crafted-document negative tests would protect this critical security boundary against regressions.
  • A test for importWallet re-checking the activation context (requireWalletImportActivationContextStillActive) when the session changes mid-flight.
  • The Solana private-key ambiguity guard at WalletImport.swift:96-98 (rejecting 32/64-char strings) has non-obvious behavior; a focused test documenting the intended rejection would help.

📝 Minor

  • WalletImport.swift:95-98: the Solana .solana(String) path decodes base58 to validate length but then returns Data(trimmed.utf8) (the base58 text), while .solanaBytes returns raw bytes. The asymmetry is intentional (server-side format expectations) but a brief comment would aid future maintainers.
  • No shell/sh code blocks with spaces-in-paths were introduced in README.md, so the path-quoting concern does not apply here. ✅

Verdict

No blocking security issues. Recommend fixing the duplicate @available (trivial) and adding at least one attestation-failure test before merge given the security-critical nature of that code path.
align-smart-sessions-solana-wallet-import

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b655a29b35

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +393 to +399
} else {
options = feeOptions.enumerated().map { index, feeOption in
FeeOptionWithBalance(
feeOption: feeOption,
selection: FeeOptionSelection(feeOption: feeOption, index: UInt32(index))
)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Populate balances before using firstAvailable for Solana

For a non-sponsored Solana transfer with selectFeeOption: .firstAvailable, this branch creates every FeeOptionWithBalance without availableRaw. firstAvailable rejects every such option when availableRaw == nil, so the transfer always throws noFeeOptionSelected even when the wallet can pay the fee. Populate Solana balances for this path or give the predefined selector a viable Solana behavior.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed this behavior also exists in TypeScript and Kotlin: Solana fee options currently lack balance enrichment, while firstAvailable is explicitly balance-aware. I am leaving this unchanged in the Swift-only review pass because falling back to the first option could select an unaffordable fee and silently change semantics. This should be handled as a separate cross-SDK design change.

Comment thread Sources/OMSWallet/Models/WaasModelMapping.swift Outdated
Comment thread docs/error-contracts.md Outdated
Preserve unknown Swift wallet metadata, remove a duplicate availability annotation, correct the access-inspection error contract, and add focused fail-closed attestation regression coverage.
@tolgahan-arikan

Copy link
Copy Markdown
Collaborator Author

Review follow-up is in signed commit 1164465. It removes the duplicate availability annotation, preserves unknown wallet key-origin and network-family values, corrects the public inspection error contract, and adds focused fail-closed attestation tests for malformed structure, freshness, PCR0 trust, nonce binding, request/response binding, and root pinning. The intentional Solana key ambiguity rule is also covered. Full scripts/verify.sh passes with 117 tests, public API checks, pod lint, and both sample builds.

@tolgahan-arikan

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 116446590d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread Sources/OMSWallet/Attestation.swift
Comment thread Sources/OMSWallet/Attestation.swift Outdated
Comment thread Sources/OMSWallet/Clients/WalletOperations.swift Outdated
@tolgahan-arikan

Copy link
Copy Markdown
Collaborator Author

@claude review

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

Claude finished @tolgahan-arikan's task —— View job


PR Review: Align wallet sessions, Solana, and wallet import

I re-reviewed the current head (1164465 + follow-ups) with focus on the security-critical wallet-import path, Solana operations, session guards, and whether prior Codex findings were addressed. This is a well-constructed, security-conscious PR.

Prior findings — verified addressed

  • Duplicate @available removed (FeeOptionsSelector.swift:29-30 now single annotation).
  • Unknown keyOrigin preservedKeyOrigin.sdkValue maps .unknown(value) through (WaasModelMapping.swift:101-103), so a forward-compatible backend rollout no longer breaks wallet decoding. Same for NetworkFamily / WalletType / TransactionStatus.
  • Solana session guardrequireActiveSolanaWallet() checks guard let walletAddress else { throw .sessionMissing() } before the family check (WalletOperations.swift:574-584), matching the Ethereum helper and the documented .sessionMissing contract.
  • Attestation errors preserved across WebRPCAttestedSignedWaasTransport.post catches .attestationVerificationFailed and re-wraps with a transportPrefix marker (Attestation.swift:87-93) so the boundary normalizes back to non-retryable, without leaking upstream diagnostics.
  • inspectRemoteCredential removed from the protected-session row in docs/error-contracts.md; it remains an intentional unsigned public request (WalletSessions.swift:294-303).

Security review — attestation & HPKE (looks solid)

  • AttestationVerifier.verify is fail-closed with good coverage: COSE_Sign1 structure, ES384 alg, canonical-base64 round-trip on the document, freshness window (±5m), PCR measurement shape, trusted PCR0 allowlist, nonce binding, and request/response user_data binding via Sequence/1:<sha256> before signature verification (Attestation.swift:141-218).
  • importWallet seals plaintext locally and zeroes it via defer { plaintext.resetBytes(...) } (WalletImports.swift:39-40); no plaintext key is written to session storage and the Keychain P-256 credential remains non-extractable. The activation context is re-checked after the network round-trip (requireWalletImportActivationContextStillActive, WalletImports.swift:62,178-190). ✅
  • The Data(base64Encoded:)! force-unwraps (WalletImports.swift:142-143) are guarded by prior canonicalBase64 validation — safe ordering.

Observations (non-blocking)

  1. ⚠️ Nitro root selection contested (Attestation.swift:222). Codex flagged bundle.first vs bundle.last as P1; the author responded that the AWS Nitro spec orders cabundle as [ROOT, INTERM_1, ...] and the TS reference + fixture follow that ordering, so .first is correct. The current code is internally consistent with that assumption — it pins bundle.first as the root, builds [leaf] + authorities.dropFirst() for evaluation, anchors authorities[0], and asserts chain.last == rootData after SecTrustEvaluateWithError (Attestation.swift:234-246). Since this is the trust anchor for imported key material, I'd flag it as worth an explicit source-of-truth citation in a code comment (link the AWS Nitro cabundle ordering doc) so future maintainers don't "fix" it back. The WalletImportTests root-pinning case gives regression protection against an accidental flip.

  2. firstAvailable + Solana still yields noFeeOptionSelected (WalletOperations.swift:390-403). For a non-sponsored Solana transfer, execute(network: nil, walletAddress: nil, ...) takes the else branch that builds FeeOptionWithBalance without availableRaw, so firstAvailable rejects every option. The author documented this as a known cross-SDK gap (TS/Kotlin share it) and intentionally deferred it to avoid silently selecting an unaffordable fee. Reasonable to defer, but consider a short doc note on sendSolanaTransfer that firstAvailable/balance-aware selectors aren't supported for Solana yet, so callers use a custom selector or predefined selection.

  3. Solana indexer URL derivation (OMSWalletEnvironment.swift:16). trimmingCharacters(in:) only trims leading/trailing /, which is fine for the standard parsePublishableKey URL shapes but has no direct unit test asserting the derived solanaIndexerGatewayUrl. A one-line assertion for the default environment URLs would lock this in cheaply.

  4. Minor: Solana .solana(String) asymmetry (WalletImport.swift:91-99). The base58 string path validates by decoding then returns Data(trimmed.utf8) (the base58 text), while .solanaBytes returns raw bytes, plus the 32/64-char ambiguity rejection. Behavior is intentional and now test-covered; a brief inline comment on the wire-format expectation would aid maintainers.

Conventions

  • Token amounts avoid floating point (FeeOptionSelector.hasEnoughBalance compares unsigned decimals by length/value; formatTokenAmount routes through formatUnits). ✅
  • @available(macOS 12.0, iOS 15.0, *) present on new public surfaces (FeeOptionWithBalance, FeeOptionSelector, ListAccessPages, WalletClient extensions, IndexerClient). ✅
  • Validation guards (isEthereumAddressValue, isCanonicalUnsignedDecimal) are consistently applied to decoded grants/sessions. ✅

Verdict

No blocking issues found. Prior review findings are addressed with regression tests. The observations above are documentation/test hardening suggestions, with #1 (an explicit citation for the Nitro cabundle ordering) being the most valuable given the security-critical nature of the trust anchor.
align-smart-sessions-solana-wallet-import

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.

1 participant