Skip to content

Feat - Upgrade Stellar Wallet Kit#33

Open
akbarsaputrait wants to merge 2 commits into
masterfrom
feat/upgrade-stellar-wallet-kit
Open

Feat - Upgrade Stellar Wallet Kit#33
akbarsaputrait wants to merge 2 commits into
masterfrom
feat/upgrade-stellar-wallet-kit

Conversation

@akbarsaputrait

Copy link
Copy Markdown
Member

No description provided.

@vercel

vercel Bot commented May 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
intent-example Ready Ready Preview, Comment May 24, 2026 12:06pm

Request Review

@shawnmuggle

Copy link
Copy Markdown
Member

looks good to me

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

AI review: 0 P0 / 4 P1 / 3 P2 — no blocking issues found

Labeled ai-review-passed. This is advisory; a human still merges.

P0 (blocking)

None.

P1 (should fix)

  • docs/STELLAR_EXTERNAL_KIT_AND_CONNECT.md: The doc describes a major breaking change (SWK v1 → v2, npm→JSR package rename, static singleton API, stellarKit prop changing from an instance to a boolean). No corresponding source diff for packages/connectkit/src/provider/StellarContextProvider.tsx, singleton-import.ts, or walletconnect.module.ts is included/verifiable in what's shown — but the bundle-analysis diff (below) shows real code changes (e.g. StellarContextProvider.tsx render size changed from 9252→9864 bytes, and a new dynamic import edge {"uid":"14e69f07-404","dynamic":true} appears, and walletconnect.module.ts/singleton-import.ts module graph disappeared/shrank). This is a breaking public API change for SDK consumers (stellarKit prop semantics flip from "pass an instance" to "pass true"). Given payment-flow-adjacent wallet connection logic changed, confirm there's actual test coverage for the new StellarWalletsKit.init() / static singleton flow, since old behavior silently changing (e.g. an old consumer still passing an instance object to stellarKit would now get truthy-coerced and silently break) is a real regression risk. No tests are visible in this diff for the new connect/disconnect/event-subscription logic.
  • docs/STELLAR_EXTERNAL_KIT_AND_CONNECT.md: stellarKit prop changing type from StellarWalletsKit to boolean is a breaking change with no migration guard — if a consumer upgrades the SDK but not their @creit-tech package/usage, passing an old kit instance object as stellarKit will be treated as truthy and silently take the "already initialized" code path even though StellarWalletsKit.init() was never called, likely causing runtime failures. Worth flagging in the migration doc as a footgun, and ideally guarded in code (e.g. runtime type-check / warning) — not verifiable from the docs diff alone but worth calling out to the author.
  • docs/STELLAR_EXTERNAL_KIT_AND_CONNECT.md (installation section): Introducing a JSR-only dependency (jsr:@creit-tech/stellar-wallets-kit) adds supply-chain/tooling risk — JSR packages aren't installable via plain npm install without the npx jsr add wrapper or registry config, which can break CI/lockfile reproducibility if not consistently pinned across environments (pnpm-lock/package-lock vs jsr's deno.json-style manifest). Confirm CI pipeline and Renovate/Dependabot support JSR before merging this guidance.
  • packages/connectkit/bundle-analysis.html: This is a generated build artifact (bundle stats) checked into source control and diffed on every build (hashes/uids and gzip sizes change per build, as seen from 434d483f-*14e69f07-* uid churn across the entire file). This bloats the diff/history and provides no reviewable signal — recommend adding bundle-analysis.html to .gitignore and generating it only in CI artifacts, not committing it.

P2 (nice to have)

  • .codegraph/.gitignore and .cursor/rules/codegraph.mdc: New tooling/config files unrelated to the payment SDK functionality — fine to merge, but confirm these don't accidentally get bundled/shipped in the published npm package (check .npmignore/files field in package.json).
  • docs/STELLAR_EXTERNAL_KIT_AND_CONNECT.md: Good rewrite overall, but the "Migration from v1" section duplicates a lot of content already shown earlier in the doc (WalletConnect config example, provider prop example) — could be condensed with cross-references to reduce maintenance burden of keeping duplicated code samples in sync.
  • docs/STELLAR_EXTERNAL_KIT_AND_CONNECT.md: The doc no longer mentions what happens if RozoPayProvider's stellarKit prop is omitted vs. explicitly false — worth clarifying whether false is treated differently from "not passed" for symmetry with true.

ai-pr-review · claude-sonnet-5

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

ai-pr-review swept this head (5f0ad9d).

@shawnmuggle

Copy link
Copy Markdown
Member

/ai-review

@github-actions

Copy link
Copy Markdown

AI review: 0 P0 / 4 P1 / 3 P2 — no blocking issues found

Labeled ai-review-passed. This is advisory; a human still merges.

P0 (blocking)

None.

P1 (should fix)

  • docs/STELLAR_EXTERNAL_KIT_AND_CONNECT.md: This is a full doc rewrite describing a major breaking migration (SWK v1 → v2, npm → JSR registry, instance → static singleton API). There is no corresponding code diff in this PR showing the actual SDK migration (StellarContextProvider.tsx, walletconnect.module.ts, etc. are referenced in the bundle-analysis tree but not shown as changed source). If the SDK code has not actually been migrated to SWK 2.x yet, this documentation is now inconsistent with the shipped implementation and will mislead integrators (wrong package name, wrong API calls, wrong stellarKit prop type — boolean vs StellarWalletsKit instance). Please confirm the doc changes match a corresponding code migration in this PR or a merged prior PR.
  • stellarKit prop type change (documented): The doc changes the stellarKit prop from accepting a StellarWalletsKit instance to a boolean. This is a breaking API change for any downstream consumer already passing a kit instance (per the "Migration from v1" section) — worth confirming this is intentionally versioned as a major/breaking release (semver) and changelog-documented, not just doc-only.
  • JSR install instructions introduce a new untrusted registry dependency (jsr:@creit-tech/stellar-wallets-kit). No mention of pinning an exact version or verifying package integrity/provenance in the doc — for a payments SDK pulling in wallet-connection code, recommend documenting exact version pinning and checking for supply-chain risk (new registry, renamed scope @creit.tech@creit-tech — potential typosquat/hijack vector if the old npm package name is ever reused by a bad actor).
  • bundle-analysis.html: This generated artifact (a build output, not source) is committed to the repo again with regenerated hashes/uids (434d483f-*14e69f07-*) purely from a rebuild. This is noisy in diffs and shouldn't be checked into version control at all — should be gitignored and generated in CI only. Not a blocking issue but indicates a process gap (every dependency bump/rebuild will produce a huge diff like this one).

P2 (nice to have)

  • .codegraph/.gitignore and .cursor/rules/codegraph.mdc: Purely tooling/editor config additions, no functional risk. Consider confirming .codegraph/ itself (the DB/cache) is not accidentally tracked elsewhere in the repo before this gitignore was added.
  • Doc formatting: In the new STELLAR_EXTERNAL_KIT_AND_CONNECT.md, the "Why JSR?" callout could be tightened; also worth double-checking all internal doc links (e.g. ./ARCHITECTURE.md, ./TROUBLESHOOTING.md) actually exist in the repo since the referenced STELLAR_PAYOUT_IMPLEMENTATION_ANALYSIS.md link was removed in this rewrite — verify no other doc references a stale link to that removed section.
  • Minor: bundle-analysis.html is over 100k+ characters of minified JSON per diff hunk — consider using git diff --stat-friendly binary/generated-file handling (e.g., mark as linguist-generated or exclude from diffs via .gitattributes) to ease future reviews.

ai-pr-review · claude-sonnet-5

@shawnmuggle shawnmuggle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AI review found no P0 blockers for a trusted Rozo contributor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants