Skip to content

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

Merged
taylanpince merged 10 commits into
masterfrom
align-smart-sessions-solana-wallet-import
Sep 17, 2026
Merged

taylanpince merged 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 using Nitro attestation verification and standard P-256 HPKE with AES-256-GCM
  • add Solana wallet creation, signing, verification, transfers, and indexer balances
  • add wallet key origin, indexed fee-option selection, and owner-side remote-access inspection, approval, listing, usage, and revocation
  • update the public API baseline, generated API docs, README, error contracts, focused tests, and the 0.2.0 to 0.3.0 migration guide

This PR prepares and versions the SDK for the 0.3.0 release: Maven publication metadata for both modules and the installation example are set to 0.3.0. No separate version-bump PR is required.

Breaking changes and migration

The public access API now models direct credentials and remote smart sessions separately. CredentialInfo is now WalletCredential; access listing returns AccessGrant and AccessGrantPage; and revokeAccess accepts credentialId plus an optional sessionId.

Wallet now includes the required keyOrigin, WalletType includes Solana, fee selections can preserve the quoted option index, and sponsored transactions invoke custom fee selectors with an empty option list.

See MIGRATION.md for concrete 0.2.0 to 0.3.0 migration steps. No compatibility shims are included.

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 deployed measurements; callers do not construct wallet-import configuration or provide PCR values.

Generated source

  • repository: 0xsequence/waas
  • branch: integration/v1.1
  • source commit: 817701409d8d980979aa03d273538ee112663eaf
  • schema: v1-26.9.9-42835a0a
  • schema hash: 2adcb9fa810bf56d76cec4c246c328e3ccad75de
  • generator: webrpc-gen v0.37.2 with gen-kotlin v0.3.2

Pinned wallet-import PCR0 values:

  • Staging: e271fe4b26c9d58d6089b908ab713f888e6107e2cb4782ddaceea950bbec9971ccd9159e7a099bd506e04ce55c3da696
  • Production: 1935cbc713f0b43060315689e87285f6ba76bcf06f26d0719735e8d674b71e0eff71dcf77fe90ab32870ef3c954973b7, published in WaaS v1.1.0

Staging can advance between WaaS releases. PCR rotation must remain SDK-first: publish trust for the current and replacement measurements before deploying the replacement, then remove the retired measurement in a later SDK 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.

Verification

  • ./gradlew --build-cache verify
  • full verification aggregate passed: 215 tasks, including tests, lint, artifact graph, generated-client isolation, public API and docs checks, and both example builds
  • push-time Kotlin style checks 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
@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

Reviewed the security-sensitive additions (wallet-import HPKE, Nitro attestation, remote-access sessions, Solana, fee selection). Overall this is careful, well-structured work: HPKE binding to a canonical SPKI, COSE_Sign1 structural checks, PCR0 trust pinning, request/response binding via user_data, nonce binding, cert-chain validation against the pinned Nitro root, and plaintext zeroization (plaintext.fill(0)). Nice.

A few items worth addressing before merge.

Blockers / should-fix

  1. No unit test for AttestationVerifier. This is the single most security-critical new file (COSE_Sign1 parsing, ES384 signature verify, PCR0 pinning, cert-chain validation, freshness, request/response + nonce binding) and AGENTS.md flags attestation/signing as high-risk requiring regression tests. There are no negative tests proving that a tampered user_data, wrong nonce, untrusted PCR0, expired timestamp, wrong root, or mutated signature is rejected. A fixture-based test (a captured-or-synthesized attested document plus mutation cases) would materially reduce risk of a future regression silently weakening verification. Currently only the happy-path HPKE round-trip and PCR0 config validation are covered (WalletImportCryptoTest.kt). Please add focused AttestationVerifier tests.

Nits / questions

  1. Freshness window is symmetricAttestationVerifier.kt:74 uses abs(timestamp - nowMillis) <= maxAgeMillis, so a document timestamped up to 5 min in the future is accepted. Some tolerance for client clock skew is reasonable, but confirm this matches the TS reference behavior; if the intent is "not older than 5 min", an asymmetric bound (allow small future skew, larger past bound, or vice-versa) may be more correct. Nit, but worth a comment either way.

  2. Cert-chain constraint check indexingAttestationVerifier.kt:142-153 iterates listOf(leaf) + authorities and checks basicConstraints >= index - 1. Since PKIXParameters trust anchor is authorities.first() and the built chain is leaf + authorities.drop(1), the manual pathlen check runs over a slightly different list than the validated path. It looks correct in practice, but the dual representation (validated chain vs. manually-iterated listOf(leaf)+authorities) is subtle; a brief comment on why both the root is both a trust anchor and re-checked would help future readers. The keyUsage bit indices (0 = digitalSignature for leaf, 5 = keyCertSign for CAs) are correct.

  3. Solana wallet-type disambiguation by string lengthWalletImportCrypto.kt:36-38 rejects base58 strings of length exactly 32 or 64 as "ambiguous". This is a reasonable guard, but a valid 32-char base58 secret could legitimately occur; the error tells callers to use SolanaBytes, which is fine. Confirm this matches TS import semantics so behavior is consistent across SDKs.

  4. Hand-rolled Base64 (WalletImportBase64) — a custom canonical encoder/decoder is used instead of android.util.Base64 / java.util.Base64. The canonical round-trip assertion (encode(decoded) == value, line 134) is a good integrity check. Given a custom implementation on a security boundary, ensure WalletImportBase64 has direct edge-case tests (padding variants, non-canonical rejection, empty). I don't see those explicitly.

  5. signSolanaMessage distinguishes wallet type via isEthereumAddress() (WalletClient.kt:1655-1665) and passes network = "" at WalletClient.kt:2957. Works, but the empty-string network sentinel for Solana is easy to break; a small comment or named constant would help.

Style / docs

  • Public APIs generally carry KDoc; good. WalletImportPrivateKey.EthereumBytes/SolanaBytes are class (not data class) presumably to avoid ByteArray equals/hashCode pitfalls — reasonable and consistent.
  • docs/api.md (19 hits) and README.md (14 hits) cover the new import/Solana/remote-access surface, and oms-wallet-kotlin-sdk/api/public-api.txt baseline is updated — good alignment with the AGENTS.md public-API contract.
  • Error surface: new AttestationVerificationFailed code + OMSWalletAttestationException (non-retryable) are wired through withOperation/toOMSWalletException and error-contracts.md is updated. Consistent.

Summary

Solid, defense-in-depth implementation. The main gap is test coverage for AttestationVerifier (and the custom Base64) given how security-critical that path is. Everything else is nits/confirmations. I'd recommend adding attestation negative-path tests before merge.

  • Wallet-import crypto (HPKE, AES-GCM)
  • Nitro attestation verification (PCR0, cert chain, COSE)
  • WalletClient auth/signing/session changes
  • Solana + indexer additions
  • Remote-access session APIs
  • Test coverage review
  • Code style + KDoc + API docs alignment

Job run • branch align-smart-sessions-solana-wallet-import

@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:22.541117Z 7455139 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.

@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: fde4fa4486

ℹ️ 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 oms-wallet-kotlin-sdk/src/main/java/technology/polygon/omswallet/OMSWallet.kt Outdated
Exercise canonical Base64 validation and fail-closed attestation checks, document the intentional Solana key ambiguity guard, and correct the access-inspection error contract.
@tolgahan-arikan

Copy link
Copy Markdown
Collaborator Author

Review follow-up is in signed commit 7455139. It adds focused fail-closed attestation tests, direct canonical Base64 padding and rejection vectors, regression coverage for the intentional Solana key ambiguity rule, and corrects the public inspection error contract. Compatibility overloads and the legacy wallet-type fallback remain intentionally excluded to match the agreed breaking scope and TypeScript behavior. Full ./gradlew --build-cache verify passes across all 215 tasks.

@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: 7455139d05

ℹ️ 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".

@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

Re-reviewed at the current HEAD, focusing on what changed since the earlier Claude/Codex passes: the attestation error-preservation fix (2a87f9d), the sponsored-transaction acknowledgement (7f2255e), and the security-critical import/attestation path.

Verdict

The two prior blockers are resolved. AttestationVerifier now has fail-closed negative tests (WalletImportCryptoTest.kt:72-148) covering malformed COSE, freshness, untrusted PCR0, nonce mismatch, request/response binding, and non-Nitro root; and attestation failures are unwrapped at the WebRPC boundary (OMSWalletError.kt attestationFailure()), so OMSWalletAttestationException survives instead of degrading to a retryable OMS_REQUEST_FAILED. Both are the right fixes. No blockers from me.

Confirmations (correct as written)

  • Attestation binding — nonce and Sequence/1:<sha256(preimage)> user_data checks (AttestationVerifier.kt:87-94) bind the document to the exact request/response, and the leaf key verifies the COSE Signature1 structure with ES384. Chain is pinned to the Nitro root by SHA-256 (:125). Solid defense-in-depth.
  • Error unwrappingattestationFailure() is applied in both runOMSWalletOperation's WebRpcTransportException catch and toOMSWalletException, and re-tags the operation when needed. Good, symmetric coverage.
  • Sponsored fee acknowledgement — invoking selectFeeOption?.select(emptyList()) and discarding the result (WalletClient.kt:2388) lets a selector veto (throw) a sponsored tx without being able to attach a fee; tests cover firstAvailable, empty-options, and throw-to-abort. Reasonable.

Nits / questions (non-blocking)

  1. Sponsored selector: return value silently discarded. At WalletClient.kt:2388 the selector result is intentionally ignored (fee is always null for sponsored). The KDoc on FeeOptionSelector documents this ("returning null acknowledges the free fee, while throwing stops execution"), which is good — but a caller who returns a non-null FeeOptionSelection for a sponsored tx gets it silently dropped rather than an error. Matching TS is fine; just confirm that's the intended contract and not a footgun.

  2. signSolanaMessage empty-network sentinel (prior review nit, still open) — WalletClient.kt:1655-1665 distinguishes wallet type via !isEthereumAddress() and the underlying call passes network = "". Works, but the empty-string sentinel remains easy to break silently. A named constant would help; low priority.

  3. Attestation freshness is symmetric (prior nit, acknowledged) — abs(timestamp - nowMillis) <= 5min (AttestationVerifier.kt:74) accepts up to 5 min of future skew. The test only exercises the past bound (:91). If TS is symmetric, fine; a one-line comment on the intent would prevent future "tightening" regressions.

  4. PCR0 measurement size vs. pinned length. WalletImportConfiguration pins PCR0 as a 96-hex (48-byte) value, but the per-PCR validation accepts 32/48/64-byte measurements (AttestationVerifier.kt:81). PCR0 specifically must be 48 bytes to match a trusted entry, so a wrong-size PCR0 is caught later by the trust check at :86 — correct, just slightly indirect. No change needed.

Style / docs

  • New public surface (import, Solana, remote-access, fee-by-index) is reflected in docs/api.md, README.md, public-api.txt, and error-contracts.md. KDoc present on new public members. Consistent with the AGENTS.md public-API/error-contract contracts.
  • Intentional breaking-scope decisions (no legacy constructor overload, no networkFamilytype fallback, Solana string-length ambiguity rule) are documented in-thread and matched to TS/Swift. Those are product/cross-SDK calls, appropriately deferred.

Summary

This is careful, well-tested defensive work and the previously-flagged blockers are addressed. Remaining items are nits/confirmations, none blocking. Nice work on the attestation negative-path coverage.

  • Read security-critical files (AttestationVerifier, WalletImportCrypto)
  • Review attestation error preservation fix (2a87f9d)
  • Review sponsored transaction commit (7f2255e)
  • Review WalletClient session/Solana/fee changes
  • Review error contracts + tests
  • Post review feedback
    • branch align-smart-sessions-solana-wallet-import

@taylanpince
taylanpince merged commit cdfecd5 into master Sep 17, 2026
5 checks passed
@tolgahan-arikan
tolgahan-arikan deleted the align-smart-sessions-solana-wallet-import branch September 17, 2026 14:29
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.

2 participants