Unblock CI and the demo suite: lint, custody key parsing, multi-step timeout, Palisade clawback - #35
Merged
Merged
Conversation
`eslint . --max-warnings 0` has been failing since 2e7cf1f, which also prevented format:check and build from running in CI at all. Two warnings, one of them a real defect: - assertClawbackEnabled had no JSDoc because its docstring was orphaned above assertRequireAuthEnabled's own docstring — the latter was added later and inserted below the existing block. Move the existing prose down to the function it documents rather than writing a new one. - Drop a redundant `no-bitwise` disable in the IOU tests. no-bitwise is already off for all of test/** (eslint.config.mjs), so the directive was reported as unused.
The contract-tests job fails both Ripple Custody sandbox tests with "Unsupported or unrecognized private key algorithm", which points at the key's curve. The key is fine: re-parsing a known-good prime256v1 PEM with its newlines replaced by literal backslash-n reproduces the failure exactly, so the value never parsed and the algorithm could not be detected. A PEM is inherently multi-line but travels through GitHub Actions secrets, .env files, JSON secret fields and shell exports, all of which routinely escape its newlines. Normalize rather than making every operator rediscover this: - resolveSigningKey strips one layer of wrapping quotes (an otherwise valid quoted PEM matches no prefix and gets treated as a file path) and restores escaped newlines. - Apply the same normalization to a PEM read from Secrets Manager; a key in a JSON string field has the identical hazard. - Split KeypairService.fromPrivateKey's error in two, so "could not parse" no longer reports as an unsupported algorithm. Conflating them is what sent this investigation at the curve. Note this makes the SDK resilient; it does not fix the CI secret, which still needs re-provisioning with its newlines intact.
Four defects surfaced by exercising the public API from outside the package. - Export `Wallet` as a value from the public surface. `LocalSigner.create()` accepts `Wallet` instances, so a caller could not use the documented factory (or generate an ad-hoc keypair) without adding a direct `xrpl` dependency for a type this SDK already requires. Importing it from the package entry point failed outright. - Preflight `Token.destroy()` against the issuance's outstanding amount. The ledger already refuses this as `tecHAS_OBLIGATIONS`, but that code names neither the issuance nor the amount, so the caller is left to guess that a holder still has a balance. - ESLint now ignores `demos/`. `.gitignore:37` explicitly invites that directory for local scratch scripts, but it belongs to no tsconfig project, so the type-aware parser errored on every file in it — creating the directory the repo suggests broke `npm run lint` outright. - Document in .env.example that both testnet seeds are required, and point at a fallback faucet. A drained account surfaces as an unrelated-looking submission failure rather than "out of funds". Not addressed here: `Token.list` returns raw base units while `Token.transfer` takes scaled display units, so a transfer of 100 at assetScale 2 reads back as 10000. Fixing that is an API semantics decision, not a bug fix.
Omitting `ticker` reached `currency.length` inside encodeCurrencyCode and surfaced as a bare "TypeError: Cannot read properties of undefined (reading 'length')" — naming neither the field, the method, nor the vertical. Found by calling the built package the way a JavaScript consumer would. TypeScript callers cannot reach this, since `ticker` is typed `string`. But the SDK publishes CJS + ESM for a Node audience that includes plain JS, and every other validation path here already fails with a named IntentValidationError (see iouValue, and Token.issue's XLS-89 report). The guard widens the value to `unknown` first: the declared type says it cannot be undefined, which is precisely why the compiler would otherwise reject the check that catches the callers who pass nothing.
The three value-transfer verbs disagreed on what to call the recipient: xrp.transfer and token.transfer take `to`, while iou.transfer took `destination`. Because the parameter shapes also differ, reaching for the wrong one surfaces as an unrelated TypeError rather than an unknown-property error. Rename IOUTransferParams.destination and IOUTransferIntent.destination to `to`. All three transfer verbs now agree. `destination` is deliberately kept where it is not a transfer — Account.fund, Account.activate and the credential verbs — so the split is now meaningful rather than accidental: `to` moves value, `destination` names an account being set up. Breaking, and free to do now: the package is 0.0.0 and unreleased. Verified against live testnet — test/integration/iou.test.ts passes (5 tests, 148s), covering transfer, offers and holder authorization.
`Palisade transaction <id> REJECTED` gave the caller nothing to act on — not the operation, not a reason — and re-reading the transaction needs credentials the caller may not have (GetTransaction returns 403 for the configured sandbox credentials). Include the action and any attributes Palisade attached. On the sandbox this turns a bare REJECTED into `... REJECTED (action=PALISADE_MANAGED)`, which at least identifies the operation class. Found while testing IOU clawback: Palisade rejects the AccountSet that enables asfAllowTrustLineClawback / asfRequireAuth, so clawback cannot be enabled on a Palisade-held issuer at all. That is separate from, and compounds, the Amount.issuer/holder mapping problem already noted.
… polling A step of a multi-step operation is not one transaction among many, it is a barrier: nothing after it runs until it lands. On a governed custodian that wait includes a human approval, and consecutive steps can belong to *different* accounts (IOU.issue sequences issuer -> holder -> issuer), so an approver may not know the next step is queued behind theirs. At the 60s single-step default the common outcome was step one timing out and the rest never being submitted, leaving a half-configured issuer. runMultiStep now defaults each step to MULTI_STEP_STEP_TIMEOUT_MS (1 hour). It applies with `??`, so an explicit per-step timeout still wins. The plumbing already existed — SubmitRequest.timeoutMs reaches both governed custodians — it simply was never set. The timeout alone would have been a bad trade. Palisade derived its poll count as timeoutMs / 1500ms and never slowed down, so an hour meant ~2,400 requests per step and ~7,200 for a three-step issuance: a rate-limiting problem swapped in for a timeout one. All three polling loops now share a backing-off schedule (initial interval, doubling, capped at 30s), which keeps the first seconds responsive and costs ~100 requests for a full hour. Palisade's loop also moves from a fixed attempt count to a deadline, since with a growing delay "number of polls" no longer tracks elapsed time, and the caller's budget is expressed in time. Verified against live testnet: test/integration/iou.test.ts passes (5 tests, 165s), covering the multi-step IOU issuance path.
Two independent problems, both observed against the Palisade sandbox: 1. There is no correct field for the holder. XRPL carries the account being clawed from in Clawback.Amount.issuer — counter-intuitive, but it is the protocol convention and what IOU.clawback builds. Palisade's SubmitClawback instead takes a separate `holder`, documented in its spec as "Optional holder address for MPTokens", while Palisade rejects MPT amounts outright. So the holder either lands in a field Palisade reads as the issuer, or in one scoped to a token type it will not accept. 2. Clawback cannot be enabled at all. asfAllowTrustLineClawback must be set before the issuer owns any trust line, and Palisade rejects that AccountSet (REJECTED action=PALISADE_MANAGED, no reason exposed through the API), reproduced against a genuinely fresh issuer wallet. Dropping the transactor from PALISADE_NATIVE_TRANSACTORS routes Clawback to raw signing where the custodian allows it, and otherwise fails with a clear capability error — rather than silently submitting a request whose holder is in the wrong field. mapClawback is deleted rather than left dead. Tests: the MPT-rejection case reached toCurrencyAmount through Clawback, so it now asserts that boundary directly — the transactors that still call it (OfferCreate, TrustSet) are typed to exclude MPT amounts. Note the comment marking the omission deliberately avoids quoting 'Clawback': scripts/gen-connector-routing.mjs scrapes quoted strings out of that Set literal, comments included, and a quoted mention put the transactor straight back into the generated routing table. Re-enable only once Palisade confirms both a field that carries an IOU clawback's holder, and that the enabling AccountSet is accepted.
pdp2121
approved these changes
Aug 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.