wallets: remove @stellar/stellar-sdk dependency#1688
wallets: remove @stellar/stellar-sdk dependency#1688albertoelias-crossmint merged 5 commits intomainfrom
Conversation
Replace Stellar SDK usage with tweetnacl (already a dep) for ed25519 operations and a local StrKey encoder ported from open-signer. Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
🦋 Changeset detectedLatest commit: 6d025e5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Original prompt from Alberto Elias
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/wallets/src/utils/stellar.ts
Line: 74-85
Comment:
**Missing deterministic test vectors for `encodeStellarPublicKey`**
The PR description explicitly acknowledges this gap: the test suite only validates the output format (`/^G[A-Z0-9]+$/`) but never compares the output against a known-good address generated by the Stellar SDK. This is a meaningful risk for a custom re-implementation of a cryptographic encoding function (CRC16-XModem + base32 + StrKey version byte).
A subtle byte-ordering bug, off-by-one in the CRC, or wrong version byte would produce a Stellar-looking string that still passes the regex test yet would be a different (invalid) address than the real SDK would generate — silently causing all server-signer-derived Stellar addresses to be wrong in production.
Please add at least one hardcoded test vector that compares output against a pre-computed Stellar SDK address, e.g.:
```typescript
// In server-signers.test.ts (or a new stellar.test.ts)
import { ed25519KeypairFromSeed, encodeStellarPublicKey } from "../../utils/stellar";
it("produces an address identical to the Stellar SDK for a known seed", () => {
// Seed: 32 bytes of 0x01
const seed = new Uint8Array(32).fill(1);
const { publicKey } = ed25519KeypairFromSeed(seed);
const address = encodeStellarPublicKey(publicKey);
// Replace with the known-good value produced by:
// Keypair.fromRawEd25519Seed(Buffer.from(seed)).publicKey()
expect(address).toBe("GCEZWKCA5VLDNRLN3RPRJMRZOX3Z6G5CHCGCHWLC8V2GCEF4OZMPOPBT");
});
```
Similarly, the `StellarServerSigner.signMessage` test only checks that the result is decodable as base64 — unlike the Solana test which actually verifies the signature with `nacl.sign.detached.verify`. It would be valuable to verify the Stellar signature is cryptographically valid against the public key.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/wallets/package.json
Line: 32
Comment:
**Leftover blank line**
Removing the `@stellar/stellar-sdk` entry left an extra blank line between `@solana/web3.js` and `abitype`.
```suggestion
"abitype": "1.0.8",
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "chore: add changeset..." |
🔥 Smoke Test Results✅ Status: Passed Statistics
✅ All smoke tests passed!All critical flows are working correctly. This is a non-blocking smoke test. Full regression tests run separately. |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/wallets/src/utils/stellar.ts
Line: 65-67
Comment:
**Stellar StrKey should not include RFC 4648 `=` padding**
The Stellar StrKey spec and SDK strip base32 padding characters — Stellar addresses never include `=`. For ed25519 public keys the payload is always 35 bytes (1 version + 32 key + 2 CRC), which encodes to exactly 56 chars (`35 × 8 / 5 = 56`, no remainder), so this code path is currently unreachable and the output is correct.
However, if `encodeStellarPublicKey` is ever called with a non-32-byte key (or if this `base32Encode` helper is reused for other Stellar key types such as seed keys or hash types), the `=` padding would be appended and produce an address that is incompatible with the Stellar protocol. Removing the padding loop makes this a correct Stellar-specific base32 encoder regardless of input size.
```suggestion
return output;
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "Apply suggestion fro..." |
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/wallets/src/signers/server/server-signers.test.ts
Line: 125-132
Comment:
**Missing cryptographic signature verification in Stellar test**
The Stellar signing test only confirms the result is decodable as base64, but does not verify the signature is actually valid against the public key. The Solana signing test (line 104) does this correctly:
```ts
expect(nacl.sign.detached.verify(messageBytes, sigBytes, pubkey)).toBe(true);
```
An equivalent check here would confirm that the `secretKey` stored in `StellarServerSigner` genuinely corresponds to the derived public key and that `ed25519Sign` produces a verifiable signature. Since `ed25519KeypairFromSeed` is now directly importable, this is straightforward to add:
```ts
import { ed25519KeypairFromSeed } from "../../utils/stellar";
it("signs a base64-encoded message", async () => {
const messageBytes = Buffer.from([1, 2, 3, 4]);
const message = messageBytes.toString("base64");
const result = await signer.signMessage(message);
expect(result.signature).toBeDefined();
const sigBytes = Buffer.from(result.signature, "base64");
const { publicKey } = ed25519KeypairFromSeed(config.derivedKeyBytes);
expect(nacl.sign.detached.verify(messageBytes, sigBytes, publicKey)).toBe(true);
});
```
Without this, the test would still pass even if the signing function returned a completely wrong or random byte sequence that happened to be base64-decodable.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "fix: remove duplicat..." |
Description
Removes the
@stellar/stellar-sdkdependency from@crossmint/wallets-sdkby replacing its two usages with lightweight alternatives already available in the codebase:tweetnacl(already a dependency, used bySolanaServerSigner)open-signer/shared/cryptography/src/primitives/encoding/strkey.tsThe Stellar SDK was only used in two files (
stellar-server-signer.tsandderive-server-signer.ts) for three operations:Keypair.fromRawEd25519Seed(),keypair.publicKey(), andkeypair.sign(). This PR replaces all three with a newpackages/wallets/src/utils/stellar.tsutility module.Key things for reviewers to verify:
encodeStellarPublicKeyimplementation (CRC16-XModem checksum + base32 encoding) produces addresses identical to the Stellar SDK. The existing test suite validates the format (/^G[A-Z0-9]+$/) and passes, but there are no hardcoded test vectors comparing against a known SDK-generated address. Consider verifying with e.g.Keypair.fromRawEd25519Seed(Buffer.from(new Uint8Array(32).fill(1))).publicKey()→"GCEZWKCA5VLDNRLN3RPRJMRZOX3Z6G5CHCGCHWLC8V2GCEF4OZMPOPBT".tweetnacl.sign.detachedproduces signatures compatible with what the Stellar SDK'sKeypair.signproduced (both are standard ed25519).@stellar/stellar-sdk(grep confirmed only these two files).Updates since last revision
abitypekey inpackage.json(introduced when the leftover blank line was removed via a GitHub suggestion that replaced it withabitypeinstead of deleting the line)Test plan
@crossmint/wallets-sdkpass, includingStellarServerSignertests (address format, signing, transaction delegation) andderiveServerSignerAddresstests for Stellar chainpnpm lint)Package updates
@stellar/stellar-sdk(v14.0.0-rc.3) frompackages/wallets/package.json@crossmint/wallets-sdk: patch)Link to Devin session: https://crossmint.devinenterprise.com/sessions/32a29a66027548e39212f6d15a5a8104
Requested by: @albertoelias-crossmint