Skip to content

Add Apple OIDC defaults and improve examples#56

Merged
taylanpince merged 9 commits into
masterfrom
improve-oidc-auth-and-examples
Jul 2, 2026
Merged

Add Apple OIDC defaults and improve examples#56
taylanpince merged 9 commits into
masterfrom
improve-oidc-auth-and-examples

Conversation

@tolgahan-arikan

@tolgahan-arikan tolgahan-arikan commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add Apple as a default OIDC provider alongside Google, with default Apple Services ID, openid email scopes, response_mode=form_post, and PKCE auth-code mode.
  • Clean up redirect auth APIs so convenience start flows use signInWithOidcRedirect and callbacks complete through completeOidcRedirectAuth.
  • Update React, Wagmi, and Trails examples for Google/Apple OIDC, shared example UI, and simpler fee/default handling.
  • Improve the Trails Earn positions layout and keep first-available fee option controls visible.
  • Align README/API/connector docs with the new OIDC and connector defaults.

Breaking Change / Migration Note

  • signInWithOidcRedirect is now start-and-navigate only and returns Promise<void>. Apps should call completeOidcRedirectAuth() on the callback page to complete redirect auth.

Validation

  • pnpm exec vitest run tests/oidcRedirectAuth.test.ts
  • pnpm exec vitest run tests/walletErrors.test.ts
  • pnpm exec vitest run tests/errorContracts.test.ts
  • pnpm test
  • pnpm exec tsc --noEmit
  • pnpm test:types
  • pnpm build
  • pnpm build:example
  • pnpm build:wagmi-example
  • pnpm build:trails-actions-example
  • pnpm build:node-example
  • pnpm build:node-contract-deploy-example
  • git diff --check

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown

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


PR Review: Add Apple OIDC defaults and improve examples

  • Review SDK behavior changes (walletClient.ts, oidc.ts, oidcRedirect.ts, omsEnvironment.ts)
  • Verify test coverage for changed behaviors
  • Check public API changes reflected in API.md and type-tests
  • Verify explicit .js extensions in relative imports under src/
  • Check for committed secrets/credential values
  • Assess TypeScript soundness / unsafe casts

Overall this is a solid, well-tested change. The SDK behavior changes are coherent, public API surface is consistently reflected in API.md, README.md, and type-tests/oidcProviderTypes.ts, and test coverage tracks each new behavior. A few observations and minor questions below.

Correctness — SDK behavior

Good: session-clearing ordering fix. Moving await this.clearSession() to after requireRedirectAuthStorage() and provider/redirectUri resolution (walletClient.ts:534-546) fixes a real footgun: previously an active session was wiped even when the storage preflight was about to throw. The regression test "does not clear an existing session when redirect storage preflight fails" (tests/oidcRedirectAuth.test.ts) locks this in.

Good: PKCE conditional + usePkce. buildOidcAuthorizationUrl now deletes scope/code_challenge params when absent (src/utils/oidcRedirect.ts:89-101), and startOidcRedirectAuth passes usePkce: authMode === AuthMode.AuthCodePKCE (walletClient.ts:590). The auth-code (non-PKCE) test verifies caller-supplied code_challenge/scope authorize params are stripped — nice defensive coverage.

completeOidcRedirectAuth early-return path (walletClient.ts:~618):

if (!hasCallbackParams && params.callbackUrl === undefined) {
    return undefined
}

This is correct for the browser "landed on a non-callback page" case (tested). Note the asymmetry: if a caller explicitly passes a callbackUrl that has no OIDC params, it does not early-return and instead proceeds to loadPendingOidcRedirectAuth, throwing "No pending OIDC redirect auth found". That's a reasonable contract, but it's an implicit behavior worth a one-line comment or a dedicated test, since the void return is now part of the public type.

cleanUrl default inversion (walletClient.ts:~624):

if (params.cleanUrl ?? (params.callbackUrl === undefined)) {

Cleaning defaults to true only when callbackUrl is omitted (auto browser mode), and to false when an explicit callbackUrl is passed. This matches the documented behavior in API.md. Both branches are tested. Good — just flagging it's a subtle default that reviewers should be aware of.

Test coverage

Coverage is strong and behavior-focused. New/updated tests cover: Apple provider defaults, Apple query redirect flow, auth-code (non-PKCE) start+complete, browser-default redirectUri, browser-default callbackUrl + cleaning, void return outside a callback, loginHint: '' suppression, start-time walletSelection/sessionLifetimeSeconds persistence, the provider-required guard, and the session-preservation regression. This aligns with the AGENTS.md "test the promise, not the implementation" guidance.

Minor gap: consider a test asserting that an explicit callbackUrl with no OIDC params throws "No pending OIDC redirect auth found" (the asymmetry noted above), so that contract is pinned.

Public API / docs

  • src/index.ts correctly exports appleOidcProvider + AppleOidcProviderParams, and StartOidcRedirectAuthParams/CompleteOidcRedirectAuthParams/SignInWithOidcRedirectParams remain exported.
  • API.md accurately reflects: optional redirectUri/callbackUrl, the new | void return, authMode on provider configs, the new AuthMode section, removal of cleanUrl/replaceUrl from signInWithOidcRedirect, and the Apple defaults.
  • type-tests/oidcProviderTypes.ts adds sound @ts-expect-error cases. Since provider is required in SignInWithOidcRedirectParams (walletClient.ts:211), the no-arg / provider-less @ts-expect-error assertions are valid.
  • signInWithOidcRedirect was narrowed from 4 overloads to a single Promise<void> — this is a breaking change to its return type (it no longer completes auth). It's well documented in README/API.md, but callers relying on the old completion-on-callback behavior must migrate to completeOidcRedirectAuth(). Worth calling out explicitly in release notes.

.js extensions / imports

All new/changed relative imports under src/ use explicit .js extensions — verified via diff scan (no violations).

Secrets

No new secrets introduced by this PR. The changed default Google client ID and the Apple Services ID (service.oms.polygon.technology) are public OAuth client identifiers, not secrets. The VITE_TRAILS_API_KEY value in examples/wagmi/.env.example predates this PR (last touched in #39) and is read from env in code (examples/wagmi/src/config.ts), not hardcoded — outside this PR's scope.

TypeScript soundness

Types are sound. New guards isOidcAuthMode, isWalletSelectionBehavior, and hasOidcRedirectStartProvider (walletClient.ts:~1886) properly narrow persisted/untyped state. The parsed.signerKeyType as CredentialSigningAlgorithm cast is pre-existing pattern, not new. hasOidcRedirectStartProvider is a runtime guard that's technically redundant with the compile-time required provider, but it's cheap defensive validation for JS callers — acceptable.

Summary

No blocking issues. Recommendations are optional: (1) add a test/comment for the explicit-callbackUrl-without-params path, and (2) highlight the signInWithOidcRedirect return-type breaking change in release notes.
• Branch: improve-oidc-auth-and-examples

Copy link
Copy Markdown
Collaborator Author

Reminder before merge/deploy: update the GitHub Pages OMS_PUBLISHABLE_KEY value for the deployed examples. Do not put the key in source; update the Pages/Actions environment configuration.

@tolgahan-arikan tolgahan-arikan marked this pull request as ready for review July 2, 2026 09:01

@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: 00d211b862

ℹ️ 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 src/clients/walletClient.ts
@taylanpince taylanpince merged commit 57a92fb into master Jul 2, 2026
4 checks passed
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