Skip to content

No clippy in CI — add it with -D warnings and clear the resulting lints across the contract #261

Description

@Jagadeeshftw

Priority: Medium  ·  Area: CI / code quality  ·  Est. effort: 6–9 h

📌 Problem

.github/workflows/ci.yml runs cargo fmt --all -- --check, cargo build and cargo test. grep -c clippy .github/workflows/ci.yml returns 0.

Formatting is enforced; correctness lints are not. On a Soroban contract the lints that matter are not stylistic — clippy flags arithmetic that can wrap, lossy numeric casts, comparison chains that admit an off-by-one at a threshold, and needless clones and allocations that cost real wasm bytes. This contract handles anchors, liquidity and settlements, so threshold and arithmetic errors are correctness bugs with value attached.

The wasm size is currently unmeasured (tracked separately), which makes allocation-related lints doubly worth having.

🎯 Design decision required

State and defend:

  1. Scope of this PR. Run it first and report the count by category. Clear everything, or enable a defined subset now and file the rest? Either is fine; picking silently is not. If you scope down, list precisely what was deferred.
  2. Allow policy. Where you keep an #[allow], is it justified by Soroban's constraints (no-std, determinism, size) or by convenience? Say which per site. A crate-level blanket #![allow] to make the job pass will be rejected.
  3. Test targets. --all-targets lints the 305 tests in src/test.rs too, which will likely dominate the count. Decide whether test targets are in scope now and argue it.

🧩 Requirements and context

  • Clippy must run with -D warnings so the job fails. A step that only prints is not worth adding.
  • No behavioural changes. If a lint fix would change behaviour, stop and note it in the PR instead.
  • Report the wasm byte delta for anything affecting generated code.
  • rust-toolchain.toml pins the toolchain — lint against it, and note that findings differ across clippy versions.
  • The Makefile has fmt/fmt-check targets; consider adding a clippy target so local and CI invocations cannot diverge.

🛠️ Suggested execution

  1. Run cargo clippy --all-targets -- -D warnings; paste the count grouped by category before fixing anything.
  2. Fix by category, one commit each.
  3. Add the CI step (and a Makefile target if you argued for it).
  4. Show it green and prove it fails on a deliberate violation.
  5. Report the wasm byte delta.

✅ Acceptance criteria

  • The PR opens with the pre-fix lint inventory by category and count.
  • cargo clippy -- -D warnings exits 0 for your stated scope.
  • A clippy step exists in CI, proven to fail on a deliberate violation (link the run).
  • Every surviving #[allow] is narrowly scoped with a justification comment.
  • No crate-level blanket #![allow] was added.
  • cargo test still passes (305 tests).
  • Wasm byte delta reported; anything deferred is listed.

🚫 Out of scope

  • Behavioural changes.
  • The wasm build and size guard — separate issue.
  • Splitting test.rs — separate issue.

🧪 Verification

cargo clippy --all-targets -- -D warnings
cargo test
cargo fmt --all -- --check
cargo build --target wasm32-unknown-unknown --release

📤 What your PR must include

  1. The pre-fix lint inventory by category.
  2. Your scope and test-target decisions.
  3. Justification per surviving #[allow].
  4. Link to a failing run, plus the wasm delta.
  5. Closes #<n>.

🔒 Security notes

Several default clippy lints map directly onto contract-safety concerns: wrapping arithmetic, lossy casts, and boundary comparisons that admit an off-by-one. On liquidity and settlement paths that arithmetic decides how much value moves, and nothing currently flags it.

📋 Guidelines

  • Minimum 95% test coverage on changed lines
  • Clear documentation
  • Timeframe: 96 hours from assignment
  • One logical change per commit; no merge commits

💬 Join our community

Working on this, or want to sanity-check your approach before you start? Come and ask — the maintainers are there and happy to help.

Telegram: https://t.me/Grainlify

Activity

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

Metadata

Metadata

Assignees

Labels

GrantFox OSSGrantFox open-source programMaybe RewardedGrantFox: potentially rewarded contributionThird CampaignGrantFox third campaign issuepriority:mediumMedium difficulty / self-contained but non-trivial

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions