Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions docs/issue-833-state-dependency-graph.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# Soroban State Dependency Graph (#833)

## Problem
Optimization and security rules currently reason about Soroban contract source
line-by-line. There is no structural model of how `env.storage()` reads,
writes, and the computations between them relate, so rules cannot tell
whether a write is derived from a prior read, or whether two storage keys
are correlated. This blocks safe, non-local optimizations (#834-#836 depend
on it).

## Prior Art In This Repo
- `src/analysis/dependency-graph/` builds a **contract-level** graph
(`SorobanDependencyAnalyzer`) tracking cross-contract calls, not state.
- `src/graphs/stellar/call-graph/` (`CallGraph`, `CallGraphNode`) builds a
**function-level** call graph — same layout to mirror at state level.
- `packages/rules/soroban/src/storage-rent-check.ts` parses storage calls
line-by-line; the pattern the new analyzer feeds for storage-aware rules.

## Design
New module: `src/analysis/state-graph/` (sibling to `dependency-graph/`,
following the same `index.ts` + `types.ts` + `*.spec.ts` layout).

**Types** (`src/analysis/state-graph/types.ts`):
```ts
export interface StateNode {
id: string; kind: 'read' | 'write' | 'computation';
storageType?: 'persistent' | 'instance' | 'temporary';
key?: string; functionName: string; line: number;
}
export interface StateEdge {
source: string;
target: string;
kind: 'reads-into' | 'writes-from' | 'computed-from';
}
export interface StateDependencyGraph {
nodes: StateNode[];
edges: StateEdge[];
}
```

**Entry point:** `StateDependencyGraphBuilder.build(filePath, source): StateDependencyGraph`,
exported from `src/analysis/state-graph/index.ts`, consumed by
`packages/rules/soroban/src/` rules and by the dataflow analyzer in #834.

## Acceptance Criteria
- [ ] `StateDependencyGraph` type and builder exist under `src/analysis/state-graph/`
- [ ] Reads (`.get`) and writes (`.set`) become distinct node kinds
- [ ] Edges connect writes to the reads/computations they depend on per function
- [ ] Graph object is importable by rule modules (no CLI/reporting coupling)
50 changes: 50 additions & 0 deletions docs/issue-834-dataflow-analysis.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# Soroban Dataflow Analysis (#834)

## Problem
Existing Soroban rules (e.g. `packages/rules/soroban/src/storage-rent-check.ts`,
`rules/stellar/optimization/detect-inefficient-symbol-usage.ts`) match local
regex patterns on single lines. They cannot detect optimizations that span
multiple statements — e.g. a value defined in one line, copied in another,
and never used in a third. This issue asks for a proper def-use dataflow
analyzer that #835 and #836 build their detections on top of.

## Prior Art In This Repo
- `src/analysis/state-graph/` (#833) already models storage reads/writes as a
graph; dataflow analysis extends this to **local variables**, not just
storage.
- `src/graphs/stellar/call-graph/call-graph-generator.ts` shows the existing
pattern for a `*-generator.ts` class producing a typed graph with a
`.spec.ts` alongside it — the dataflow analyzer should follow the same
shape.

## Design
New module: `src/analysis/dataflow/` (`dataflow-analyzer.ts`, `types.ts`,
`dataflow-analyzer.spec.ts`).

**Types** (`src/analysis/dataflow/types.ts`):
```ts
export interface DefUseEntry {
variable: string;
definedAt: { line: number; functionName: string };
uses: Array<{ line: number; kind: 'read' | 'arg' | 'return' }>;
sourceKind: 'literal' | 'param' | 'storage-read' | 'call-result' | 'copy';
}
export interface DataflowResult {
entries: DefUseEntry[];
unusedDefinitions: DefUseEntry[]; // uses.length === 0
}
```

**Entry point:** `SorobanDataflowAnalyzer.analyze(source: string): DataflowResult`,
exported from `src/analysis/dataflow/index.ts`. Walks `let`/`let mut`
bindings, function parameters, and `return` statements using the same
line-scanning approach as `storage-rent-check.ts`, tracking simple
straight-line control flow (branches treated as separate paths, no loop
fixpoint required for v1 per the issue's "basic control-flow paths"
requirement).

## Acceptance Criteria
- [ ] `DataflowResult` produced per-function with defs and uses populated
- [ ] Variable definitions, uses, function args, and return values all tracked
- [ ] `unusedDefinitions` correctly flags a def with zero recorded uses
- [ ] `dataflow-analyzer.spec.ts` covers straight-line and simple branching code
50 changes: 50 additions & 0 deletions docs/issue-835-unused-computation-results.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# Detect Unused Soroban Computation Results (#835)

## Problem
Contract functions sometimes compute a value (arithmetic, a method call, a
storage read) and bind it to a variable that is never subsequently read,
returned, or passed as an argument. Each such computation still consumes gas
on-chain with zero effect. This needs to be flagged as a lint rule, built on
top of the dataflow analyzer from #834.

## Prior Art In This Repo
- Rule shape: `packages/rules/soroban/src/storage-rent-check.ts` defines
`SorobanStorageRentCheckRule` with a `public static readonly RULE_ID`
and a `.analyze(sourceCode): Warning[]` method, registered via
`packages/rules/soroban/src/index.ts` (`export * from './...'`).
- Warning shape: `StorageRentWarning` in the same file (`line`, `message`,
`suggestion` fields) is the field convention this rule's output should
match.

## Design
New file: `packages/rules/soroban/src/unused-computation-check.ts`,
registered by adding `export * from './unused-computation-check';` to
`packages/rules/soroban/src/index.ts`.

```ts
export interface UnusedComputationWarning {
line: number;
variable: string;
message: string;
suggestion: string;
}

export class SorobanUnusedComputationRule {
public static readonly RULE_ID = 'soroban-unused-computation';
public analyze(sourceCode: string): UnusedComputationWarning[] { /* ... */ }
}
```

**Inputs:** `DataflowResult` from `src/analysis/dataflow/` (#834), specifically
its `unusedDefinitions` list, filtered to `sourceKind !== 'param'` (function
parameters that go unused are a separate, existing concern) and
`sourceKind !== 'literal'` (cheap constants aren't worth flagging).
**Outputs:** one `UnusedComputationWarning` per unused non-trivial definition,
with `suggestion` telling the developer to remove the binding or use `let _ =`.

## Acceptance Criteria
- [ ] `SorobanUnusedComputationRule.RULE_ID = 'soroban-unused-computation'`
- [ ] Rule consumes `DataflowResult.unusedDefinitions`, not its own regex scan
- [ ] Storage reads and call-results with zero uses are reported; literals and
unused fn params are excluded
- [ ] Each warning includes source `line` and an actionable `suggestion`
50 changes: 50 additions & 0 deletions docs/issue-836-unnecessary-variable-copies.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# Detect Unnecessary Soroban Variable Copies (#836)

## Problem
Rust/Soroban code sometimes clones or reassigns a value into a new binding
that is used only once, immediately after, in place of the original
(`let b = a.clone(); do_thing(b);` where `a` is otherwise unused after). This
adds avoidable clone/copy overhead. The detector must be conservative: types
with real ownership semantics (`Address`, moved `Vec`/`Map`/`Bytes`) must not
be flagged unless the copy is provably redundant, per the issue's explicit
"avoid ownership-related false positives" requirement.

## Prior Art In This Repo
- Rule shape and registration follow `packages/rules/soroban/src/storage-rent-check.ts`
/ `packages/rules/soroban/src/index.ts`, same as #835.
- Data source: the def-use chains from `src/analysis/dataflow/` (#834) —
a "copy chain" is simply a `DefUseEntry` whose `sourceKind === 'copy'`
(i.e. `let x = y;` or `let x = y.clone();`) where the source variable `y`
has no further uses after the copy point.

## Design
New file: `packages/rules/soroban/src/unnecessary-copy-check.ts`, registered
via `packages/rules/soroban/src/index.ts`.

```ts
export interface UnnecessaryCopyWarning {
line: number;
copiedVariable: string;
originalVariable: string;
message: string;
suggestion: string;
}

export class SorobanUnnecessaryCopyRule {
public static readonly RULE_ID = 'soroban-unnecessary-copy';
public analyze(sourceCode: string): UnnecessaryCopyWarning[] { /* ... */ }
}
```

**Detection rule (conservative v1):** only flag `let x = y.clone();` /
`let x = y;` where (a) `y` has no uses in the dataflow graph after the copy
line within the same function, and (b) `y`'s type is not one of the
Soroban-owned handle types (`Address`, `Env`, `BytesN`) obtained from a
function parameter — parameters are excluded entirely for v1 to avoid
ownership false positives, matching the issue's explicit acceptance bar.

## Acceptance Criteria
- [ ] `SorobanUnnecessaryCopyRule.RULE_ID = 'soroban-unnecessary-copy'`
- [ ] Copy chains derived from `src/analysis/dataflow/` def-use data, not regex
- [ ] Function-parameter-sourced values are never flagged (false-positive guard)
- [ ] `.spec.ts` covers a flagged clone-then-unused case and a not-flagged reused case
Loading