Align wallet sessions, Solana, and wallet import - #45
tolgahan-arikan wants to merge 10 commits into
Conversation
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.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @tolgahan-arikan's task —— View job PR Review: Align wallet sessions, Solana, and wallet importReview todo list:
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
|
There was a problem hiding this comment.
💡 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".
| } else { | ||
| options = feeOptions.enumerated().map { index, feeOption in | ||
| FeeOptionWithBalance( | ||
| feeOption: feeOption, | ||
| selection: FeeOptionSelection(feeOption: feeOption, index: UInt32(index)) | ||
| ) | ||
| } |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
Preserve unknown Swift wallet metadata, remove a duplicate availability annotation, correct the access-inspection error contract, and add focused fail-closed attestation regression coverage.
|
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. |
|
@codex review |
There was a problem hiding this comment.
💡 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".
|
@claude review |
|
Claude finished @tolgahan-arikan's task —— View job PR Review: Align wallet sessions, Solana, and wallet importI re-reviewed the current head ( Prior findings — verified addressed
Security review — attestation & HPKE (looks solid)
Observations (non-blocking)
Conventions
VerdictNo 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 |
Summary
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
WalletImportConfigurationor 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
The generated transport remains internal, so upstream backend operations are not part of the public SDK surface.
Generated source
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
WalletImportConfigurationdoes not break an existing released API. No compatibility shim or release-version bump is included.Pre-merge requirement
stagingWalletImportPcr0swith 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.productionWalletImportPcr0swith 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.Do not merge until these checks are complete. A wrong pin either blocks wallet import or trusts an unintended enclave image.
Verification
scripts/verify.shgit diff --checkpassed