Mid term fixes - #108
Conversation
…rit/StablePay into add-tectonic-adapter
|
Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. 📝 WalkthroughWalkthroughThe pull request adds repository governance and branding, a local Tectonic protocol and SDK, protocol adapters, local network support, adapter-backed StablePay payment flows, tests, deployment tooling, and demo configuration. ChangesRepository foundation
Local Tectonic protocol stack
StablePay adapter integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PaymentWidget
participant Transaction
participant ProtocolAdapter
participant TectonicClient
participant TectonicContract
PaymentWidget->>Transaction: Request native payment quote
Transaction->>ProtocolAdapter: quoteNativePayment
ProtocolAdapter->>TectonicClient: quotePayment
TectonicClient->>TectonicContract: Read price and fee state
TectonicContract-->>TectonicClient: Return quote inputs
TectonicClient-->>PaymentWidget: Return required payment
PaymentWidget->>Transaction: Build mint transaction
Transaction->>ProtocolAdapter: buildMintTx
ProtocolAdapter->>TectonicClient: buildMintTx
TectonicClient-->>PaymentWidget: Return encoded transaction
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (16)
brand/scripts/generate-rasters.py (1)
102-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove comments that only label the following calls.
PWA install iconsandMulti-resolution .icorestate the save operations. Keep the comments that explain padding, platform behavior, and rendering constraints.As per coding guidelines, comments must “Explain reasoning and constraints, not syntax.”
Also applies to: 110-114
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@brand/scripts/generate-rasters.py` around lines 102 - 104, Remove the redundant label comments immediately preceding the PWA icon saves and the multi-resolution .ico saves, including the comments around the symbols save and render calls; preserve comments that document padding, platform behavior, or rendering constraints.Source: Coding guidelines
tectonic-local/test/Tectonic.t.sol (1)
386-400: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test can pass without asserting anything.
The assertion sits inside
if (tectonic.ratio() < CRITICAL_RATIO). When the sweep restores the ratio, the test body runs no assertion at all. Assert the stated disjunction directly, so both outcomes are checked.♻️ Proposed strengthening
_depressRatioBelowCritical(); tectonic.forceRedemptions(10); - if (tectonic.ratio() < CRITICAL_RATIO) { - assertEq(tectonic.holderCount(), 0, "stopped below critical with holders remaining"); - } + // Either the sweep restored the ratio, or it consumed every holder. + bool restored = tectonic.ratio() >= CRITICAL_RATIO; + bool exhausted = tectonic.holderCount() == 0; + assertTrue(restored || exhausted, "sweep stopped early with holders still redeemable");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tectonic-local/test/Tectonic.t.sol` around lines 386 - 400, Update test_ForcedRedemptionsStopOnlyWhenRestoredOrExhausted so it always asserts the intended disjunction: after forceRedemptions, the ratio is restored to at least CRITICAL_RATIO or holderCount() is zero. Remove the conditional wrapper so both possible outcomes are validated.tectonic-local/foundry.toml (1)
45-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the Forge version and correct the stale rationale.
No Forge version is pinned, so lint-ID resolution remains version-dependent. Pin or document the supported Forge version before relying on these seven exclusions. Update
tectonic-sdk/TectonicABI.jsontotectonic-sdk/src/artifacts/TectonicABI.js. Align the patch counts withTectonic.sol, which lists patches 1–9 plus 3b.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tectonic-local/foundry.toml` around lines 45 - 54, Update the lint configuration around the seven entries in [lint].exclude_lints to pin or document the supported Forge version, ensuring lint-ID resolution is stable; correct the stale rationale comments, update the ABI artifact reference from tectonic-sdk/TectonicABI.json to tectonic-sdk/src/artifacts/TectonicABI.js, and align patch-count references with Tectonic.sol’s patches 1–9 plus 3b.tectonic-local/.gitignore (1)
8-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the Foundry dependency versions.
lib/is ignored andfoundry.lockis absent, so the reinstall command can resolve different revisions.Tectonic.solrequires OpenZeppelin Contracts 5.x throughutils/ReentrancyGuard.soland_update. Replace both unpinned dependencies with valid explicit tags, or commit a dependency lockfile.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tectonic-local/.gitignore` around lines 8 - 11, Update the Foundry dependency setup referenced by the lib/ ignore rule to ensure reproducible installs: replace both unpinned forge install dependencies with explicit valid version tags compatible with Tectonic.sol, including OpenZeppelin Contracts 5.x, or add and commit the appropriate foundry.lock file.stablepay-sdk/src/widget/TransactionReview.jsx (1)
178-179: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare
paidSymbolwithconst.
paidAmountis reassigned on the native path at line 193, soletis correct for it.paidSymbolis never reassigned.♻️ Proposed change
let paidAmount = contextTransactionDetails.amount; - let paidSymbol = selectedToken.symbol; + const paidSymbol = selectedToken.symbol;As per coding guidelines: "ES modules everywhere;
constoverlet; novar".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stablepay-sdk/src/widget/TransactionReview.jsx` around lines 178 - 179, Change the paidSymbol declaration in the transaction review flow to const because it is never reassigned, while keeping paidAmount as let since the native path updates it.Source: Coding guidelines
stablepay-sdk/example/src/App.jsx (1)
28-45: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize
merchantConfigandnetworkSelector.Both objects are constructed in the component body, so each render produces new identities.
TransactionReviewlistsnetworkSelectorin its initialization effect dependencies, so a new identity rebuildsTransaction, re-runsinit(), and re-issues the quote and warning RPC reads.Appholds no state today, so the demo does not hit this often, but the coupling is easy to trip later.♻️ Proposed refactor
-function App() { +function App() { + const merchantConfig = useMemo( + () => + new StablePay.Config({ + receivingAddress: isLocalTectonic + ? '0x70997970C51812dc3A010C7d01b50e0d17dc79C8' + : '0x000000000000000000000000000000000000dEaD', + amounts: isLocalTectonic + ? { 'tectonic-local': { stablecoin: 5 } } + : { + 'sepolia': { stablecoin: 5 }, + 'milkomeda-mainnet': { stablecoin: 5 }, + 'ethereum-classic': { stablecoin: 5 }, + }, + blacklist: isLocalTectonic ? [11155111, 2001, 61] : [31337], + }), + [] + ); + + const networkSelector = useMemo( + () => new StablePay.NetworkSelector(merchantConfig), + [merchantConfig] + );Add the import:
+import { useMemo } from 'react'Keep the existing explanatory comments on the receiving address, amounts, and blacklist when you move them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stablepay-sdk/example/src/App.jsx` around lines 28 - 45, Memoize the StablePay.Config instance and the StablePay.NetworkSelector instance in App so their identities remain stable across renders, while preserving the existing receivingAddress, amounts, and blacklist comments and configuration. Update the relevant imports and ensure the selector memoization reuses the memoized merchantConfig.stablepay-sdk/src/core/adapters/TectonicAdapter.js (1)
109-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the Tectonic scaling factor instead of hardcoding
1e18.Line 110 hardcodes the fixed-point divisor.
tectonic-sdk/src/constants.jsowns this scale, and the repository guidelines call out that Tectonic scales by1e24... no — by1e18, while Djed uses1e24. If the constant ever changes, this literal drifts silently and the displayed fee percentage becomes wrong.Import the exported scale from
tectonic-sdkand divide by it.♻️ Proposed refactor
-import { TectonicClient, RESERVE_HEALTH, fromBaseUnits } from "tectonic-sdk"; +import { TectonicClient, RESERVE_HEALTH, fromBaseUnits, SCALE } from "tectonic-sdk";- const dailyPercent = (Number(health.dailyStabilityFeeRate) / 1e18) * 100; + const dailyPercent = (Number(health.dailyStabilityFeeRate) / Number(SCALE)) * 100;Adjust the imported name to match the actual export in
tectonic-sdk/src/constants.js.As per coding guidelines: "Djed scales by
1e24. Tectonic scales by1e18. The two SDKs deliberately do not share the constant — see the comment at the top oftectonic-sdk/src/constants.js."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stablepay-sdk/src/core/adapters/TectonicAdapter.js` around lines 109 - 120, Update the fee calculation in TectonicAdapter’s RESERVE_HEALTH.FEE_ACCRUING branch to import and use the exported Tectonic scaling constant from tectonic-sdk instead of the hardcoded 1e18 divisor. Match the import name to the actual export in tectonic-sdk/src/constants.js and preserve the existing percentage formatting.Source: Coding guidelines
stablepay-sdk/src/utils/config.js (1)
122-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for
useLocalTectonic.
useLocalTectonicis now part of the public export surface instablepay-sdk/src/index.js. It validates the address format and mutates two fields of a shared module-level object. Add a test that covers a valid address, an invalid address, and the resultingtectonicAddress/tokens.stablecoin.addressvalues.As per coding guidelines: "New functionality ships with tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stablepay-sdk/src/utils/config.js` around lines 122 - 129, Add tests for the public useLocalTectonic function covering a valid address, an invalid address that throws, and verification that both tectonicAddress and tokens.stablecoin.address are updated to the valid address on the returned shared configuration object.Source: Coding guidelines
stablepay-sdk/src/widget/TokenDropdown.jsx (1)
37-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
constwith a conditional expression.The repository guideline prefers
constoverlet. A ternary removes the reassignment.As per coding guidelines: "ES modules everywhere;
constoverlet; novar".♻️ Proposed change
- let quote = null; - if (newValue === "native") { - quote = await transaction.quoteNativePayment(String(tokenAmount)); - } + const quote = + newValue === "native" + ? await transaction.quoteNativePayment(String(tokenAmount)) + : null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stablepay-sdk/src/widget/TokenDropdown.jsx` around lines 37 - 40, Update the quote initialization in the TokenDropdown payment flow to use a const conditional expression instead of let with reassignment, preserving the native-token quote behavior and the existing null value for other token types.Source: Coding guidelines
stablepay-sdk/src/contexts/chains.js (2)
142-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
localChain.rpcUrlsinstead of repeating the URL.Line 151 repeats
http://127.0.0.1:8545, which line 83 already declares. Thesepoliabranch readssepolia.rpcUrls.default.http. Follow the same pattern so the two definitions cannot drift.♻️ Proposed change
- rpcUrls: ['http://127.0.0.1:8545'], + rpcUrls: localChain.rpcUrls.default.http,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stablepay-sdk/src/contexts/chains.js` around lines 142 - 155, Update the `tectonic-local` branch in the chain configuration to set `rpcUrls` from the existing `localChain.rpcUrls` value, matching the reuse pattern used by the `sepolia` branch, and remove the duplicated hardcoded URL.
72-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove
network: 'localhost'if no compatibility consumer requires it. No repository code readslocalChain.network; this is an optional cleanup, not a confirmed viem type error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stablepay-sdk/src/contexts/chains.js` around lines 72 - 87, Remove the optional network property from the localChain definition unless an external compatibility consumer requires it; no repository code uses localChain.network, so keep the remaining defineChain configuration unchanged.stablepay-sdk/src/core/adapters/DjedAdapter.js (1)
85-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the default UI fee beneficiary address into config.
Line 87 hardcodes a fallback beneficiary address in adapter code. Any network entry that omits
uiFeeAddresssilently routes fees to this address. Declare the default next to the network entries instablepay-sdk/src/utils/config.js, or requireuiFeeAddressexplicitly for Djed networks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stablepay-sdk/src/core/adapters/DjedAdapter.js` around lines 85 - 90, The buildMintTx method in DjedAdapter must not hardcode a fallback UI fee beneficiary. Define the default uiFeeAddress alongside the relevant network entries in config.js, or require it for every Djed network, then have buildMintTx use the configured value without an adapter-level fallback.tectonic-sdk/src/tectonic.js (2)
87-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAttach the original error as
cause.The catch block formats a diagnostic message but discards the error object. The stack and viem metadata are lost, which makes RPC and decode failures harder to diagnose. Keep the message and pass
cause.♻️ Proposed refactor
} catch (error) { throw new Error( `Failed to read Tectonic contract at ${this.address}.\n\n` + `Possible causes:\n` + `- The address is not a Tectonic contract\n` + `- The contract is not deployed on this chain\n` + `- The RPC endpoint is unreachable\n\n` + - `Underlying error: ${error?.shortMessage || error?.message || error}` + `Underlying error: ${error?.shortMessage || error?.message || error}`, + { cause: error } ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tectonic-sdk/src/tectonic.js` around lines 87 - 96, Update the error construction in the catch block around the Tectonic contract read to preserve the caught error as the new Error’s cause while retaining the existing diagnostic message and underlying-error details. Use the caught error from the catch binding without changing the surrounding failure handling.
210-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
BC_DECIMALSinstead of the literal18.These three call sites hard-code the basecoin decimals while the stablecoin path correctly uses
this.params.decimals ?? SC_DECIMALS.BC_DECIMALSalready exists insrc/constants.jsline 14 for this purpose. The behaviour is correct today, so this is a maintainability fix.♻️ Proposed refactor
import { D, + BC_DECIMALS, SC_DECIMALS, GAS_LIMIT_MULTIPLIER_PERCENT, RESERVE_HEALTH, } from "./constants.js";- requiredBCFormatted: fromBaseUnits(requiredBC, 18, 8), + requiredBCFormatted: fromBaseUnits(requiredBC, BC_DECIMALS, 8),- const value = typeof amountBC === "bigint" ? amountBC : toBaseUnits(amountBC, 18); + const value = typeof amountBC === "bigint" ? amountBC : toBaseUnits(amountBC, BC_DECIMALS);- payoutBCFormatted: fromBaseUnits(payoutBC, 18, 8), + payoutBCFormatted: fromBaseUnits(payoutBC, BC_DECIMALS, 8),Also applies to: 219-219, 238-238
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tectonic-sdk/src/tectonic.js` at line 210, Replace the hard-coded 18 argument in each affected requiredBCFormatted calculation with the existing BC_DECIMALS constant, including the call sites corresponding to lines 219 and 238. Import or reuse BC_DECIMALS from src/constants.js without changing the existing formatting behavior.tectonic-sdk/scripts/smoke-local.mjs (1)
30-32: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the anvil key overridable and refuse non-local RPC endpoints.
Static analysis flags line 31 as a hard-coded key. The value is the public anvil account
#0key, so it is not a secret. The risk is reuse: the literal is unconditional, andRPC_URLalready accepts any endpoint, so this script can sign against a non-local chain with a checked-in key.Read the key from the environment with the anvil default as a fallback, and reject a non-local RPC host.
🔒️ Proposed change
// anvil's deterministic accounts `#0` and `#1`. -const PAYER_KEY = "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"; +// Public anvil test key, kept as a default so the script runs with no setup. +// Override it for any other chain; never reuse it outside a local devnet. +const PAYER_KEY = + process.env.PAYER_PRIVATE_KEY ?? + "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80"; const MERCHANT = "0x70997970C51812dc3A010C7d01b50e0d17dc79C8"; + +if (!process.env.PAYER_PRIVATE_KEY && !/^https?:\/\/(127\.0\.0\.1|localhost)(:|\/|$)/.test(RPC)) { + console.error(`Refusing to sign with the public anvil key against ${RPC}.`); + process.exit(1); +}I could not run this script, because it requires a running anvil node and a broadcast deployment.
The coding guidelines state: "Never introduce a real key, an RPC URL with an embedded API token, or a
.envfile."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tectonic-sdk/scripts/smoke-local.mjs` around lines 30 - 32, Update the smoke script’s PAYER_KEY configuration to read an environment override while retaining the existing deterministic Anvil account `#0` value as the fallback, and validate RPC_URL before signing so only a local Anvil host is accepted. Reject non-local endpoints early, preserving the existing local execution flow for approved hosts.Sources: Coding guidelines, Linters/SAST tools
tectonic-sdk/test/client.test.js (1)
177-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the redeem path and
getState.The suite covers mint, payment, transfer, health, and balances.
quoteRedeem,buildRedeemTx, andgetStateare public API and have no test.buildRedeemTxalso contains thereceiver ?? fromdefault, which is untested branching logic.The coding guidelines state: "New functionality ships with tests."
Do you want me to generate the missing test cases?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tectonic-sdk/test/client.test.js` around lines 177 - 202, Add tests covering the public redeem flow and state accessors alongside the existing client tests: exercise quoteRedeem, buildRedeemTx, and getState with representative inputs and assertions on their results. Include separate buildRedeemTx coverage for an explicit receiver and for the receiver ?? from fallback, verifying the generated transaction uses the expected recipient.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BestPracticesChecklist.md`:
- Around line 3-14: Remove the blank line separating the attribution blockquote
from the following content in BestPracticesChecklist.md, so the blockquote is
not split and the Purpose and Legend sections remain normal Markdown content.
In `@brand/Brand.md`:
- Around line 215-221: Correct the “Regenerating” documentation to state that
generate-rasters.py maintains a duplicated CIRCLES definition and does not read
SVG geometry. Instruct contributors to update both the script’s geometry and the
SVG assets whenever the logo geometry changes.
In `@brand/scripts/generate-rasters.py`:
- Around line 85-126: Add tests for main and the asset-generation helpers that
run against a temporary output directory, verifying all expected filenames, PNG
dimensions and modes, and the ICO frames at 16, 32, 48, and 64 pixels. Ensure
the test isolates generated files from the repository’s FAV directory and covers
the complete written asset set.
In `@SECURITY.md`:
- Around line 50-52: Update the “Supported versions” section in SECURITY.md to
align with the package versions documented in BestPracticesChecklist.md:
describe security support separately for stablepay-sdk and djed-sdk, or remove
the inaccurate pre-1.0 statement while preserving the existing release-support
policy.
In `@stablepay-sdk/example/vite.config.js`:
- Around line 28-32: Update the server.fs.allow configuration in the Vite config
to replace the monorepo-root entry with explicit paths for the SDK source and
only the workspace sibling packages imported by the example. Keep the allowlist
limited to those required package directories and do not permit the repository
root.
In `@stablepay-sdk/package.json`:
- Around line 15-18: Align the distribution model for djed-sdk and tectonic-sdk
across stablepay-sdk/package.json lines 15-18 and
stablepay-sdk/rollup.config.mjs lines 15-28: for a local package, add "private":
true; for a published package, replace both file: dependencies with registry
versions or workspace references. Then make the Rollup external and globals
configuration match that choice by externalizing both SDKs or bundling both.
In `@stablepay-sdk/src/core/adapters/DjedAdapter.js`:
- Around line 68-83: Update quoteNativePayment to retain the full trade result,
construct requiredBC from BigInt(r.totalBCUnscaled) instead of totalBCScaled,
and keep totalBCScaled only for display. Adjust requiredBCFormatted precision to
match the other adapter’s eight-decimal presentation where required.
In `@stablepay-sdk/src/core/adapters/ProtocolAdapter.js`:
- Around line 21-23: Complete the documentation sentence in the ProtocolAdapter
amount-boundary comment so it explicitly states that invoices must preserve
exact 18-decimal precision, using bigint base units rather than floating-point
numbers.
In `@stablepay-sdk/src/core/Transaction.js`:
- Around line 129-137: Update _decorateConnectionError so codes 4001 and -32005
are excluded from the connection-error classification; reserve that
classification for -32603 and the existing connection-related message checks,
while preserving the original errors and their appropriate handling for user
rejection and rate limiting.
In `@stablepay-sdk/src/utils/config.js`:
- Around line 86-109: Update the default configuration for the `tectonic-local`
network so it is included in the blacklist used by `NetworkSelector`, while
preserving its existing local-network settings and `useLocalTectonic()`
behavior. Ensure both user-facing network lists exclude it by default without
affecting explicitly enabled local usage.
In `@stablepay-sdk/src/widget/TransactionReview.jsx`:
- Around line 56-93: Add a cancellation flag to the effect that creates and
initializes Transaction, and return cleanup that marks the run inactive. Before
every asynchronous state update—setTransaction, setTradeDataBuySc,
setProtocolWarnings in the getWarnings chain, and setTransactionDetails—verify
the run is still active so stale network or token selections cannot overwrite
current state. Ensure deferred warning callbacks also honor the flag.
In `@tectonic-local/script/DeployLocal.s.sol`:
- Around line 34-43: Update run() to validate the connected chain ID before
vm.startBroadcast(pk) or deploying MockOracle and Tectonic. Allow only the
intended local Anvil chain ID, and revert with a clear error for any other RPC
network.
In `@tectonic-local/src/Tectonic.sol`:
- Around line 388-409: Guard both equity coin paths against ecPrice() returning
zero before performing arithmetic or burning tokens: in mintEquityCoins, revert
before amountRC division; in redeemEquityCoins, revert before calculating value
or burning the caller’s coins. Reuse the same guard treatment and established
error behavior as yieldFromStabilityFeeDaily, and add tests that drive E() to
zero before exercising both entry points.
- Around line 253-266: Update _redeem to settle the stability fee before
calculating the redemption amount, so the burn uses the post-fee balance;
document that callers may redeem at most balanceOfAfterStabilityFee. In
_forceRedemptions, settle/read each holder’s post-fee balance before calling
_redeem while preserving the backfill check. Add a regression test that warps
time below rsafe and redeems the holder’s full post-fee balance.
- Around line 275-314: Update _forceRedemptions and its forced-redemption payout
path to use trySend instead of strict send: when a holder cannot receive funds,
record the redemption amount as claimable credit rather than reverting, and
provide or reuse a withdrawal flow for that credit. Apply the same non-reverting
trySend behavior to the tx.origin refund, while preserving strict send for
caller-initiated redemptions through _redeem.
In `@tectonic-sdk/scripts/smoke-local.mjs`:
- Around line 34-35: Validate the --amount argument where INVOICE is
initialized, handling a missing value after --amount with a clear usage error
instead of allowing undefined to reach client.quoteMint. Use let only if needed
to preserve the existing const-first convention, and keep valid supplied amounts
unchanged.
In `@tectonic-sdk/src/pricing.js`:
- Around line 44-57: Centralize fee-scale validation across all pricing
consumers: in pricing.js lines 44-57, update stablecoinsForPayment to call
netFactor(fees) before its separate floor divisions; in pricing.js lines 93-100,
update payoutForRedemption to call netFactor(fees) before subtracting fees; in
pricing.test.js lines 158-171, extend the REGRESSION test to assert both
functions throw /consume the entire payment/ for djedScaledFee.
---
Nitpick comments:
In `@brand/scripts/generate-rasters.py`:
- Around line 102-104: Remove the redundant label comments immediately preceding
the PWA icon saves and the multi-resolution .ico saves, including the comments
around the symbols save and render calls; preserve comments that document
padding, platform behavior, or rendering constraints.
In `@stablepay-sdk/example/src/App.jsx`:
- Around line 28-45: Memoize the StablePay.Config instance and the
StablePay.NetworkSelector instance in App so their identities remain stable
across renders, while preserving the existing receivingAddress, amounts, and
blacklist comments and configuration. Update the relevant imports and ensure the
selector memoization reuses the memoized merchantConfig.
In `@stablepay-sdk/src/contexts/chains.js`:
- Around line 142-155: Update the `tectonic-local` branch in the chain
configuration to set `rpcUrls` from the existing `localChain.rpcUrls` value,
matching the reuse pattern used by the `sepolia` branch, and remove the
duplicated hardcoded URL.
- Around line 72-87: Remove the optional network property from the localChain
definition unless an external compatibility consumer requires it; no repository
code uses localChain.network, so keep the remaining defineChain configuration
unchanged.
In `@stablepay-sdk/src/core/adapters/DjedAdapter.js`:
- Around line 85-90: The buildMintTx method in DjedAdapter must not hardcode a
fallback UI fee beneficiary. Define the default uiFeeAddress alongside the
relevant network entries in config.js, or require it for every Djed network,
then have buildMintTx use the configured value without an adapter-level
fallback.
In `@stablepay-sdk/src/core/adapters/TectonicAdapter.js`:
- Around line 109-120: Update the fee calculation in TectonicAdapter’s
RESERVE_HEALTH.FEE_ACCRUING branch to import and use the exported Tectonic
scaling constant from tectonic-sdk instead of the hardcoded 1e18 divisor. Match
the import name to the actual export in tectonic-sdk/src/constants.js and
preserve the existing percentage formatting.
In `@stablepay-sdk/src/utils/config.js`:
- Around line 122-129: Add tests for the public useLocalTectonic function
covering a valid address, an invalid address that throws, and verification that
both tectonicAddress and tokens.stablecoin.address are updated to the valid
address on the returned shared configuration object.
In `@stablepay-sdk/src/widget/TokenDropdown.jsx`:
- Around line 37-40: Update the quote initialization in the TokenDropdown
payment flow to use a const conditional expression instead of let with
reassignment, preserving the native-token quote behavior and the existing null
value for other token types.
In `@stablepay-sdk/src/widget/TransactionReview.jsx`:
- Around line 178-179: Change the paidSymbol declaration in the transaction
review flow to const because it is never reassigned, while keeping paidAmount as
let since the native path updates it.
In `@tectonic-local/.gitignore`:
- Around line 8-11: Update the Foundry dependency setup referenced by the lib/
ignore rule to ensure reproducible installs: replace both unpinned forge install
dependencies with explicit valid version tags compatible with Tectonic.sol,
including OpenZeppelin Contracts 5.x, or add and commit the appropriate
foundry.lock file.
In `@tectonic-local/foundry.toml`:
- Around line 45-54: Update the lint configuration around the seven entries in
[lint].exclude_lints to pin or document the supported Forge version, ensuring
lint-ID resolution is stable; correct the stale rationale comments, update the
ABI artifact reference from tectonic-sdk/TectonicABI.json to
tectonic-sdk/src/artifacts/TectonicABI.js, and align patch-count references with
Tectonic.sol’s patches 1–9 plus 3b.
In `@tectonic-local/test/Tectonic.t.sol`:
- Around line 386-400: Update
test_ForcedRedemptionsStopOnlyWhenRestoredOrExhausted so it always asserts the
intended disjunction: after forceRedemptions, the ratio is restored to at least
CRITICAL_RATIO or holderCount() is zero. Remove the conditional wrapper so both
possible outcomes are validated.
In `@tectonic-sdk/scripts/smoke-local.mjs`:
- Around line 30-32: Update the smoke script’s PAYER_KEY configuration to read
an environment override while retaining the existing deterministic Anvil account
`#0` value as the fallback, and validate RPC_URL before signing so only a local
Anvil host is accepted. Reject non-local endpoints early, preserving the
existing local execution flow for approved hosts.
In `@tectonic-sdk/src/tectonic.js`:
- Around line 87-96: Update the error construction in the catch block around the
Tectonic contract read to preserve the caught error as the new Error’s cause
while retaining the existing diagnostic message and underlying-error details.
Use the caught error from the catch binding without changing the surrounding
failure handling.
- Line 210: Replace the hard-coded 18 argument in each affected
requiredBCFormatted calculation with the existing BC_DECIMALS constant,
including the call sites corresponding to lines 219 and 238. Import or reuse
BC_DECIMALS from src/constants.js without changing the existing formatting
behavior.
In `@tectonic-sdk/test/client.test.js`:
- Around line 177-202: Add tests covering the public redeem flow and state
accessors alongside the existing client tests: exercise quoteRedeem,
buildRedeemTx, and getState with representative inputs and assertions on their
results. Include separate buildRedeemTx coverage for an explicit receiver and
for the receiver ?? from fallback, verifying the generated transaction uses the
expected recipient.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dd4793e-1d96-476f-8554-9f64988a0305
⛔ Files ignored due to path filters (30)
.DS_Storeis excluded by!**/.DS_Storebrand/color/palette.svgis excluded by!**/*.svgbrand/favicon/android-chrome-192x192.pngis excluded by!**/*.pngbrand/favicon/android-chrome-512x512.pngis excluded by!**/*.pngbrand/favicon/apple-touch-icon.pngis excluded by!**/*.pngbrand/favicon/favicon-16x16.pngis excluded by!**/*.pngbrand/favicon/favicon-32x32.pngis excluded by!**/*.pngbrand/favicon/favicon.icois excluded by!**/*.icobrand/favicon/favicon.svgis excluded by!**/*.svgbrand/favicon/maskable-icon-512x512.pngis excluded by!**/*.pngbrand/favicon/og-image.pngis excluded by!**/*.pngbrand/logo/stablepay-logo-mono.svgis excluded by!**/*.svgbrand/logo/stablepay-logo.svgis excluded by!**/*.svgbrand/typography/typography.svgis excluded by!**/*.svgstablepay-sdk/dist/esm/index.jsis excluded by!**/dist/**stablepay-sdk/dist/umd/index.jsis excluded by!**/dist/**stablepay-sdk/dist/umd/index.js.mapis excluded by!**/dist/**,!**/*.mapstablepay-sdk/example/package-lock.jsonis excluded by!**/package-lock.jsonstablepay-sdk/example/public/android-chrome-192x192.pngis excluded by!**/*.pngstablepay-sdk/example/public/android-chrome-512x512.pngis excluded by!**/*.pngstablepay-sdk/example/public/apple-touch-icon.pngis excluded by!**/*.pngstablepay-sdk/example/public/favicon-16x16.pngis excluded by!**/*.pngstablepay-sdk/example/public/favicon-32x32.pngis excluded by!**/*.pngstablepay-sdk/example/public/favicon.icois excluded by!**/*.icostablepay-sdk/example/public/favicon.svgis excluded by!**/*.svgstablepay-sdk/example/public/maskable-icon-512x512.pngis excluded by!**/*.pngstablepay-sdk/example/public/og-image.pngis excluded by!**/*.pngstablepay-sdk/package-lock.jsonis excluded by!**/package-lock.jsontectonic-local/package-lock.jsonis excluded by!**/package-lock.jsontectonic-sdk/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
.coderabbit.yaml.github/workflows/scorecard.yml.gitignoreAGENTS.mdBestPracticesChecklist.mdCONTRIBUTING.mdLICENSEMAINTAINERS.mdREADME.mdSECURITY.mdbrand/Brand.mdbrand/color/palette.cssbrand/color/palette.jsonbrand/favicon/site.webmanifestbrand/scripts/generate-rasters.pybrand/typography/typography.csschecklist-status.jsonstablepay-sdk/.gitignorestablepay-sdk/example/index.htmlstablepay-sdk/example/public/site.webmanifeststablepay-sdk/example/src/App.jsxstablepay-sdk/example/vite.config.jsstablepay-sdk/package.jsonstablepay-sdk/rollup.config.mjsstablepay-sdk/src/contexts/chains.jsstablepay-sdk/src/core/Transaction.jsstablepay-sdk/src/core/adapters/DjedAdapter.jsstablepay-sdk/src/core/adapters/ProtocolAdapter.jsstablepay-sdk/src/core/adapters/TectonicAdapter.jsstablepay-sdk/src/core/adapters/index.jsstablepay-sdk/src/index.jsstablepay-sdk/src/utils/config.jsstablepay-sdk/src/widget/TokenDropdown.jsxstablepay-sdk/src/widget/TransactionReview.jsxtectonic-local/.gitignoretectonic-local/deployments/.gitkeeptectonic-local/foundry.tomltectonic-local/script/DeployLocal.s.soltectonic-local/src/Coin.soltectonic-local/src/IOracle.soltectonic-local/src/Math.soltectonic-local/src/MockOracle.soltectonic-local/src/Tectonic.soltectonic-local/test/Tectonic.t.soltectonic-sdk/.gitignoretectonic-sdk/package.jsontectonic-sdk/scripts/smoke-local.mjstectonic-sdk/src/artifacts/TectonicABI.jstectonic-sdk/src/constants.jstectonic-sdk/src/index.jstectonic-sdk/src/pricing.jstectonic-sdk/src/tectonic.jstectonic-sdk/test/client.test.jstectonic-sdk/test/pricing.test.js
| > Criteria adapted from the [OpenSSF Best Practices Badge](https://github.com/coreinfrastructure/best-practices-badge) | ||
| > (MIT / CC BY 3.0) by OpenSSF contributors. Modified for AOSSIE multi-repo template use. | ||
|
|
||
| > **Purpose:** Covers OpenSSF Best Practices criteria that are NOT auto-detected by OpenSSF Scorecard. | ||
| > Scorecard already handles: License, SAST tools, CI tests, Security Policy file, Branch Protection, | ||
| > Pinned Dependencies, Signed Releases, Maintained status, and Known Vulnerabilities. | ||
| > | ||
| > **Legend:** | ||
| > - 🔴 MUST — Required for passing | ||
| > - 🟡 SHOULD — Required unless documented rationale given | ||
| > - 🔵 SUGGESTED — Optional but recommended | ||
| > - ⚪ N/A — Marked `[~]` with justification |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the blank line inside the blockquote.
Line 5 splits one blockquote and triggers MD028. Keep the attribution as a blockquote, then make the purpose and legend normal Markdown content.
Proposed fix
> Criteria adapted from the [OpenSSF Best Practices Badge](https://github.com/coreinfrastructure/best-practices-badge)
> (MIT / CC BY 3.0) by OpenSSF contributors. Modified for AOSSIE multi-repo template use.
-> **Purpose:** Covers OpenSSF Best Practices criteria that are NOT auto-detected by OpenSSF Scorecard.
-> Scorecard already handles: License, SAST tools, CI tests, Security Policy file, Branch Protection,
-> Pinned Dependencies, Signed Releases, Maintained status, and Known Vulnerabilities.
->
-> **Legend:**
-> - 🔴 MUST — Required for passing
-> - 🟡 SHOULD — Required unless documented rationale given
-> - 🔵 SUGGESTED — Optional but recommended
-> - ⚪ N/A — Marked `[~]` with justification
+**Purpose:** Covers OpenSSF Best Practices criteria that are NOT auto-detected by OpenSSF Scorecard.
+Scorecard already handles: License, SAST tools, CI tests, Security Policy file, Branch Protection,
+Pinned Dependencies, Signed Releases, Maintained status, and Known Vulnerabilities.
+
+**Legend:**
+- 🔴 MUST — Required for passing
+- 🟡 SHOULD — Required unless documented rationale given
+- 🔵 SUGGESTED — Optional but recommended
+- ⚪ N/A — Marked `[~]` with justification📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| > Criteria adapted from the [OpenSSF Best Practices Badge](https://github.com/coreinfrastructure/best-practices-badge) | |
| > (MIT / CC BY 3.0) by OpenSSF contributors. Modified for AOSSIE multi-repo template use. | |
| > **Purpose:** Covers OpenSSF Best Practices criteria that are NOT auto-detected by OpenSSF Scorecard. | |
| > Scorecard already handles: License, SAST tools, CI tests, Security Policy file, Branch Protection, | |
| > Pinned Dependencies, Signed Releases, Maintained status, and Known Vulnerabilities. | |
| > | |
| > **Legend:** | |
| > - 🔴 MUST — Required for passing | |
| > - 🟡 SHOULD — Required unless documented rationale given | |
| > - 🔵 SUGGESTED — Optional but recommended | |
| > - ⚪ N/A — Marked `[~]` with justification | |
| > Criteria adapted from the [OpenSSF Best Practices Badge](https://github.com/coreinfrastructure/best-practices-badge) | |
| > (MIT / CC BY 3.0) by OpenSSF contributors. Modified for AOSSIE multi-repo template use. | |
| **Purpose:** Covers OpenSSF Best Practices criteria that are NOT auto-detected by OpenSSF Scorecard. | |
| Scorecard already handles: License, SAST tools, CI tests, Security Policy file, Branch Protection, | |
| Pinned Dependencies, Signed Releases, Maintained status, and Known Vulnerabilities. | |
| **Legend:** | |
| - 🔴 MUST — Required for passing | |
| - 🟡 SHOULD — Required unless documented rationale given | |
| - 🔵 SUGGESTED — Optional but recommended | |
| - ⚪ N/A — Marked `[~]` with justification |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 5-5: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@BestPracticesChecklist.md` around lines 3 - 14, Remove the blank line
separating the attribution blockquote from the following content in
BestPracticesChecklist.md, so the blockquote is not split and the Purpose and
Legend sections remain normal Markdown content.
Source: Linters/SAST tools
| ### Regenerating | ||
|
|
||
| ```bash | ||
| cd brand && python3 scripts/generate-rasters.py | ||
| ``` | ||
|
|
||
| Requires Pillow. Edit the logo geometry in one place — the script reads the same circle definitions as the SVG — and every raster stays consistent. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the regeneration instruction.
generate-rasters.py does not read the SVG geometry. It duplicates CIRCLES, and its own Lines 19-20 require synchronized edits. State that geometry changes must update both the script and SVG assets.
Proposed fix
-Requires Pillow. Edit the logo geometry in one place — the script reads the same circle definitions as the SVG — and every raster stays consistent.
+Requires Pillow. If the logo geometry changes, update `CIRCLES` and the corresponding SVG geometry together, then regenerate every raster.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Regenerating | |
| ```bash | |
| cd brand && python3 scripts/generate-rasters.py | |
| ``` | |
| Requires Pillow. Edit the logo geometry in one place — the script reads the same circle definitions as the SVG — and every raster stays consistent. | |
| ### Regenerating | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@brand/Brand.md` around lines 215 - 221, Correct the “Regenerating”
documentation to state that generate-rasters.py maintains a duplicated CIRCLES
definition and does not read SVG geometry. Instruct contributors to update both
the script’s geometry and the SVG assets whenever the logo geometry changes.
| def main(): | ||
| os.makedirs(FAV, exist_ok=True) | ||
| written = [] | ||
|
|
||
| def save(img, name): | ||
| img.save(os.path.join(FAV, name)) | ||
| written.append(name) | ||
|
|
||
| # Browser tab icons. Tight padding: at 16px every pixel of margin is a | ||
| # pixel the mark does not get. | ||
| save(render(16, pad_ratio=0.02), "favicon-16x16.png") | ||
| save(render(32, pad_ratio=0.02), "favicon-32x32.png") | ||
|
|
||
| # iOS composites the touch icon onto an opaque tile regardless, so supply | ||
| # a white one explicitly rather than letting it pick. | ||
| save(flatten(render(180, pad_ratio=0.12)), "apple-touch-icon.png") | ||
|
|
||
| # PWA install icons. | ||
| save(render(192, pad_ratio=0.06), "android-chrome-192x192.png") | ||
| save(render(512, pad_ratio=0.06), "android-chrome-512x512.png") | ||
|
|
||
| # Android crops maskable icons to the inner 80%, so pad harder and fill | ||
| # the plate; otherwise the outer discs get clipped by the OS mask. | ||
| save(flatten(render(512, pad_ratio=0.20)), "maskable-icon-512x512.png") | ||
|
|
||
| # Multi-resolution .ico for legacy browsers and pinned tabs. | ||
| render(256, pad_ratio=0.02).save( | ||
| os.path.join(FAV, "favicon.ico"), | ||
| sizes=[(16, 16), (32, 32), (48, 48), (64, 64)], | ||
| ) | ||
| written.append("favicon.ico") | ||
|
|
||
| # 1200x630 social preview, mark on white at the optical left. | ||
| og = Image.new("RGB", (1200, 630), (255, 255, 255)) | ||
| mark = render(360, pad_ratio=0.0) | ||
| og.paste(mark, (110, 135), mark) | ||
| save(og, "og-image.png") | ||
|
|
||
| for name in written: | ||
| size = os.path.getsize(os.path.join(FAV, name)) | ||
| print(f" {name:32s} {size:>8,} bytes") | ||
| print(f"\n{len(written)} files written to {FAV}") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add tests for generated assets.
This new generator has no supplied test coverage. Add a test that writes to a temporary output directory and verifies the expected filenames, image dimensions, image modes, and ICO frame sizes.
As per coding guidelines, “New functionality ships with tests.”
🧰 Tools
🪛 ast-grep (0.45.1)
[error] 94-94: Avoid HTML built in strings
Context: render(16, pad_ratio=0.02)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 95-95: Avoid HTML built in strings
Context: render(32, pad_ratio=0.02)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 99-99: Avoid HTML built in strings
Context: render(180, pad_ratio=0.12)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 102-102: Avoid HTML built in strings
Context: render(192, pad_ratio=0.06)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 103-103: Avoid HTML built in strings
Context: render(512, pad_ratio=0.06)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 107-107: Avoid HTML built in strings
Context: render(512, pad_ratio=0.20)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 110-110: Avoid HTML built in strings
Context: render(256, pad_ratio=0.02)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
[error] 118-118: Avoid HTML built in strings
Context: render(360, pad_ratio=0.0)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').
(html-string-from-parameters)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@brand/scripts/generate-rasters.py` around lines 85 - 126, Add tests for main
and the asset-generation helpers that run against a temporary output directory,
verifying all expected filenames, PNG dimensions and modes, and the ICO frames
at 16, 32, 48, and 64 pixels. Ensure the test isolates generated files from the
repository’s FAV directory and covers the complete written asset set.
Source: Coding guidelines
| ## Supported versions | ||
|
|
||
| StablePay is pre-1.0 and under active development. Security fixes land on `main` and in the next release; there are no long-term support branches yet. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Align the supported-version statement.
Line 52 says StablePay is pre-1.0. BestPracticesChecklist.md Line 76 identifies stablepay-sdk as 1.0.3 and djed-sdk as 1.0.2. This conflict can make security support expectations unclear. Describe support per package, or remove the pre-1.0 claim.
Proposed fix
-StablePay is pre-1.0 and under active development. Security fixes land on `main` and in the next release; there are no long-term support branches yet.
+StablePay packages are under active development. Security fixes land on `main` and in the next release; there are no long-term support branches yet.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Supported versions | |
| StablePay is pre-1.0 and under active development. Security fixes land on `main` and in the next release; there are no long-term support branches yet. | |
| ## Supported versions | |
| StablePay packages are under active development. Security fixes land on `main` and in the next release; there are no long-term support branches yet. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SECURITY.md` around lines 50 - 52, Update the “Supported versions” section in
SECURITY.md to align with the package versions documented in
BestPracticesChecklist.md: describe security support separately for
stablepay-sdk and djed-sdk, or remove the inaccurate pre-1.0 statement while
preserving the existing release-support policy.
| server: { | ||
| fs: { | ||
| allow: ['..'] | ||
| } | ||
| } | ||
| // Allow serving files from the SDK and its local workspace siblings. | ||
| allow: ['../..'], | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Narrow the dev-server filesystem allowlist.
server.fs.allow: ['../..'] resolves to the monorepo root relative to the example root. The Vite dev server can then serve any file below it, including a root-level .env or key material a contributor keeps locally. Only the SDK source and the workspace sibling packages need to be reachable.
🔒 Proposed narrower allowlist
server: {
fs: {
- // Allow serving files from the SDK and its local workspace siblings.
- allow: ['../..'],
+ // Allow serving files from the SDK and its local workspace siblings.
+ allow: [
+ fileURLToPath(new URL('..', import.meta.url)),
+ fileURLToPath(new URL('../../tectonic-sdk', import.meta.url)),
+ fileURLToPath(new URL('../../djed-sdk', import.meta.url)),
+ ],
},
},Adjust the sibling list to the packages the example actually imports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@stablepay-sdk/example/vite.config.js` around lines 28 - 32, Update the
server.fs.allow configuration in the Vite config to replace the monorepo-root
entry with explicit paths for the SDK source and only the workspace sibling
packages imported by the example. Keep the allowlist limited to those required
package directories and do not permit the repository root.
| function redeem(uint256 amountSC, address receiver) external nonReentrant { | ||
| _redeem(amountSC, msg.sender, receiver); | ||
| } | ||
|
|
||
| /// PATCH 2: `nonReentrant` removed. Every caller is already guarded; the | ||
| /// nested acquisition made redeem() and all forced redemptions revert. | ||
| function _redeem(uint256 amountSC, address from, address receiver) internal { | ||
| uint256 scP = scPriceRedeem(); | ||
| uint256 value = (amountSC * scP) / D; | ||
| uint256 amountBC = deductFees(value); | ||
| _burn(from, amountSC); | ||
| send(receiver, amountBC); | ||
| emit Redeemed(from, receiver, amountSC, amountBC); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Full-balance redemption reverts whenever a stability fee is owed.
_burn(from, amountSC) calls _update, which charges the stability fee first and burns part of from's balance. super._update for the outer burn then sees balance - feeAmount and reverts with ERC20InsufficientBalance.
The condition is not exotic. A fee is owed whenever ratio() <= safeReserveRatio and any time has passed since the last charge. In that state:
redeem(balanceOf(msg.sender), receiver)always reverts._forceRedemptionsreverts too, because it readsuint256 b = balanceOf(h)on line 290 and then calls_redeem(b, h, h). A reverting sweep also revertsmint,mintEquityCoinsandredeemEquityCoins, which all invoke_forceRedemptions.
Settle the fee before you size the redemption, and read the post-fee balance in the sweep. Also document that external callers must pass at most balanceOfAfterStabilityFee.
The current tests do not reach this path. test_RedeemSucceeds and test_ForcedRedemptionPaysTheHolderInBasecoin never combine vm.warp with a depressed ratio. Add a regression test that warps time below rsafe and then redeems the full balance.
🐛 Proposed fix for `_redeem` and the sweep
function _redeem(uint256 amountSC, address from, address receiver) internal {
+ // Settle the accrued fee first. _burn triggers _update, which charges the
+ // fee and would otherwise leave the balance short of amountSC.
+ chargeStabilityFee(from);
uint256 scP = scPriceRedeem();
uint256 value = (amountSC * scP) / D;
uint256 amountBC = deductFees(value);
_burn(from, amountSC);
send(receiver, amountBC);
emit Redeemed(from, receiver, amountSC, amountBC);
}Apply the matching change inside _forceRedemptions, so b reflects the post-fee balance and the backfill check still works:
- address h = holders[i];
- uint256 b = balanceOf(h);
uint256 lengthBefore = holders.length;
+ address h = holders[i];
+ chargeStabilityFee(h);
+ uint256 b = balanceOf(h);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tectonic-local/src/Tectonic.sol` around lines 253 - 266, Update _redeem to
settle the stability fee before calculating the redemption amount, so the burn
uses the post-fee balance; document that callers may redeem at most
balanceOfAfterStabilityFee. In _forceRedemptions, settle/read each holder’s
post-fee balance before calling _redeem while preserving the backfill check. Add
a regression test that warps time below rsafe and redeems the holder’s full
post-fee balance.
| function _forceRedemptions(uint256 maxIterations) internal { | ||
| uint256 n = holderCount(); | ||
| if (n == 0) return; // PATCH 3: upstream computed `% 0` and reverted | ||
|
|
||
| uint256 gasStart = gasleft(); | ||
| uint256 iterations = 0; | ||
| uint256 totalRedeemedAmountSC = 0; | ||
| uint256 initialRatio = ratio(); | ||
| // pseudo-random starting index for fairness among holders | ||
| uint256 i = 1 + (uint256(keccak256(abi.encodePacked(block.timestamp, msg.sender))) % n); | ||
| while (ratio() < criticalReserveRatio && iterations < maxIterations && gasleft() > gasStart / 2) { | ||
| if (holderCount() == 0) break; // PATCH 3: supply fully redeemed | ||
| if (i >= holders.length) i = 1; | ||
|
|
||
| address h = holders[i]; | ||
| uint256 b = balanceOf(h); | ||
| uint256 lengthBefore = holders.length; | ||
|
|
||
| if (b > 0) { | ||
| _redeem(b, h, h); | ||
| totalRedeemedAmountSC += b; | ||
| } | ||
| iterations++; | ||
|
|
||
| // PATCH 3b: updateHolder removes a holder by swapping the last | ||
| // entry into the vacated slot, so after a redemption index `i` | ||
| // holds a different, unvisited holder. Advancing unconditionally | ||
| // would pass over them for the remainder of this sweep. Coverage | ||
| // still worked out in practice because the index wraps, but | ||
| // re-examining the slot matches the evident intent and makes the | ||
| // traversal independent of the wrap arithmetic. | ||
| if (holders.length == lengthBefore) i++; | ||
| } | ||
| uint256 finalRatio = ratio(); | ||
| // refund capped at 0.1% of the total amount redeemed | ||
| uint256 refund = | ||
| Math.min((gasStart - gasleft()) * block.basefee, totalRedeemedAmountSC * scPriceRedeem() / 1000 / D); | ||
| emit ForcedRedemptions(totalRedeemedAmountSC, initialRatio, finalRatio, tx.origin, refund); | ||
| if (refund > 0 && iterations > 0) send(tx.origin, refund); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
One hostile holder can block every operation that triggers a sweep.
_redeem pays the redeemed holder through send, which reverts on a failed call. Holders are arbitrary addresses. A holder contract whose receive reverts, or that consumes the forwarded gas, makes the whole sweep revert.
mint, mintEquityCoins and redeemEquityCoins call _forceRedemptions when ratio() < criticalReserveRatio. So a single unpayable holder makes those entry points unusable in exactly the under-reserved state that forced redemption exists to repair. StablePay's payment flow depends on mint.
Make the sweep tolerate an unpayable holder. Record a claimable credit instead of reverting, and let the holder withdraw it later.
🛡️ Sketch: credit unpayable holders instead of reverting
+ mapping(address => uint256) public pendingWithdrawals;
+
+ /// Sweeps must not depend on a holder accepting basecoin. If the transfer
+ /// fails, the amount is credited and the holder withdraws it themselves.
+ function withdraw() external nonReentrant {
+ uint256 amount = pendingWithdrawals[msg.sender];
+ require(amount > 0, "Nothing to withdraw");
+ pendingWithdrawals[msg.sender] = 0;
+ send(msg.sender, amount);
+ }
+
+ function trySend(address receiver, uint256 amount) internal {
+ if (amount == 0) return;
+ (bool success,) = payable(receiver).call{value: amount}("");
+ if (!success) pendingWithdrawals[receiver] += amount;
+ }Use trySend on the forced-redemption payout path and on the tx.origin refund, and keep the strict send for caller-initiated redemptions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tectonic-local/src/Tectonic.sol` around lines 275 - 314, Update
_forceRedemptions and its forced-redemption payout path to use trySend instead
of strict send: when a holder cannot receive funds, record the redemption amount
as claimable credit rather than reverting, and provide or reuse a withdrawal
flow for that credit. Apply the same non-reverting trySend behavior to the
tx.origin refund, while preserving strict send for caller-initiated redemptions
through _redeem.
| function mintEquityCoins(address receiver) external payable nonReentrant { | ||
| if (ratio() < criticalReserveRatio) { | ||
| _forceRedemptions(numRedemptionIterations); // PATCH 2 | ||
| } | ||
| uint256 rcBP = ecPrice(); | ||
| uint256 amountBC = deductFees(msg.value); | ||
| uint256 amountRC = (amountBC * D) / rcBP; | ||
| equityCoin.mint(receiver, amountRC); | ||
| emit MintedEquityCoins(msg.sender, receiver, amountRC, msg.value); | ||
| } | ||
|
|
||
| function redeemEquityCoins(uint256 amountRC, address receiver) external nonReentrant { | ||
| require(equityCoin.balanceOf(msg.sender) >= amountRC, "redeemEquityCoin: insufficient balance"); | ||
| uint256 value = (amountRC * ecPrice()) / D; | ||
| uint256 amountBC = deductFees(value); | ||
| equityCoin.burn(msg.sender, amountRC); | ||
| send(receiver, amountBC); | ||
| emit RedeemedEquityCoins(msg.sender, receiver, amountRC, amountBC); | ||
| if (ratio() < criticalReserveRatio) { | ||
| _forceRedemptions(numRedemptionIterations); // PATCH 2 | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guard ecPrice() == 0 in both equity coin paths.
ecPrice() returns E() * D / sRC. When the reserve is fully committed to liabilities, scPriceRedeem() is capped at R() * D / totalSupply(), so L() equals R() up to flooring and E() is zero. ecPrice() then returns 0. Two consequences follow:
- Line 394 computes
amountBC * D / rcBPand reverts with panic 0x12. Equity minting is unavailable in the exact state where new equity capital is needed. - Line 401 computes
valueas 0, soamountBCis 0 andsendreturns early on line 441. Line 403 still burns the caller's equity coins. The caller loses the coins and receives nothing, with no revert.
Patch 6 already guards this division inside yieldFromStabilityFeeDaily, so apply the same treatment here.
🐛 Proposed guards
function mintEquityCoins(address receiver) external payable nonReentrant {
if (ratio() < criticalReserveRatio) {
_forceRedemptions(numRedemptionIterations); // PATCH 2
}
uint256 rcBP = ecPrice();
+ // E() == 0 makes ecPrice() zero. Fail with a signal instead of panic 0x12.
+ require(rcBP > 0, "Tectonic: equity is worthless");
uint256 amountBC = deductFees(msg.value); function redeemEquityCoins(uint256 amountRC, address receiver) external nonReentrant {
require(equityCoin.balanceOf(msg.sender) >= amountRC, "redeemEquityCoin: insufficient balance");
- uint256 value = (amountRC * ecPrice()) / D;
+ uint256 ecP = ecPrice();
+ // Never burn equity coins for a zero payout.
+ require(ecP > 0, "Tectonic: equity is worthless");
+ uint256 value = (amountRC * ecP) / D;Add tests that drive E() to zero and then call each entry point.
Also applies to: 435-438
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tectonic-local/src/Tectonic.sol` around lines 388 - 409, Guard both equity
coin paths against ecPrice() returning zero before performing arithmetic or
burning tokens: in mintEquityCoins, revert before amountRC division; in
redeemEquityCoins, revert before calculating value or burning the caller’s
coins. Reuse the same guard treatment and established error behavior as
yieldFromStabilityFeeDaily, and add tests that drive E() to zero before
exercising both entry points.
| const amountArgIndex = process.argv.indexOf("--amount"); | ||
| const INVOICE = amountArgIndex !== -1 ? process.argv[amountArgIndex + 1] : "100"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the --amount value.
If --amount is the last argument, process.argv[amountArgIndex + 1] is undefined. INVOICE then becomes undefined, and client.quoteMint(INVOICE) at line 105 throws Tectonic: "undefined" is not a valid decimal amount. That message does not name the real problem.
🐛 Proposed fix
const amountArgIndex = process.argv.indexOf("--amount");
-const INVOICE = amountArgIndex !== -1 ? process.argv[amountArgIndex + 1] : "100";
+let INVOICE = "100";
+if (amountArgIndex !== -1) {
+ INVOICE = process.argv[amountArgIndex + 1];
+ if (!INVOICE || INVOICE.startsWith("--")) {
+ console.error("--amount requires a value, for example: --amount 250");
+ process.exit(1);
+ }
+}let is required here, so keep the const-first rule for the surrounding code.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const amountArgIndex = process.argv.indexOf("--amount"); | |
| const INVOICE = amountArgIndex !== -1 ? process.argv[amountArgIndex + 1] : "100"; | |
| const amountArgIndex = process.argv.indexOf("--amount"); | |
| let INVOICE = "100"; | |
| if (amountArgIndex !== -1) { | |
| INVOICE = process.argv[amountArgIndex + 1]; | |
| if (!INVOICE || INVOICE.startsWith("--")) { | |
| console.error("--amount requires a value, for example: --amount 250"); | |
| process.exit(1); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tectonic-sdk/scripts/smoke-local.mjs` around lines 34 - 35, Validate the
--amount argument where INVOICE is initialized, handling a missing value after
--amount with a clear usage error instead of allowing undefined to reach
client.quoteMint. Use let only if needed to preserve the existing const-first
convention, and keep valid supplied amounts unchanged.
| export function stablecoinsForPayment(amountBC, scPriceMint, fees) { | ||
| const value = toBigInt(amountBC); | ||
| const price = requirePositive(scPriceMint, "scPriceMint"); | ||
| const { fee, treasuryFee } = fees; | ||
|
|
||
| // Reproduce the contract's two separate floor divisions rather than folding | ||
| // them into one: the results differ by up to 1 wei, and the merchant-facing | ||
| // guarantee depends on matching the contract exactly. | ||
| const f = (value * toBigInt(fee)) / D; | ||
| const fT = (value * toBigInt(treasuryFee)) / D; | ||
| const net = value - f - fT; | ||
|
|
||
| return (net * D) / price; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fee-scale validation is centralised in netFactor, but only one of three consumers calls it. requiredPaymentForStablecoins routes through netFactor and therefore rejects a Djed-scaled (1e24) fee. The other two pricing functions divide by D directly and return 0n or a negative bigint for the same input, and the regression test covers only the guarded path.
tectonic-sdk/src/pricing.js#L44-L57: callnetFactor(fees)instablecoinsForPaymentbefore the two floor divisions.tectonic-sdk/src/pricing.js#L93-L100: callnetFactor(fees)inpayoutForRedemptionbefore subtracting the fees.tectonic-sdk/test/pricing.test.js#L158-L171: extend theREGRESSIONtest to assert thatstablecoinsForPaymentandpayoutForRedemptionalso throw/consume the entire payment/fordjedScaledFee.
📍 Affects 2 files
tectonic-sdk/src/pricing.js#L44-L57(this comment)tectonic-sdk/src/pricing.js#L93-L100tectonic-sdk/test/pricing.test.js#L158-L171
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tectonic-sdk/src/pricing.js` around lines 44 - 57, Centralize fee-scale
validation across all pricing consumers: in pricing.js lines 44-57, update
stablecoinsForPayment to call netFactor(fees) before its separate floor
divisions; in pricing.js lines 93-100, update payoutForRedemption to call
netFactor(fees) before subtracting fees; in pricing.test.js lines 158-171,
extend the REGRESSION test to assert both functions throw /consume the entire
payment/ for djedScaledFee.
1 participant
Addressed Issues:
Fixes #(TODO:issue number)
Screenshots/Recordings:
https://drive.google.com/file/d/1ucH_TO89D-SKAs_EPYZI2shWbK4u5COd/view?usp=sharing
Additional Notes:
Added .md files and brand folder as reccomended by bruno.
Checklist
AI Usage Disclosure
Check one of the checkboxes below:
I have used the following AI models and tools: Claude Opus
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Summary by CodeRabbit
New Features
Documentation