Skip to content

refactor(crypto): remove deprecated methods and harden SM2/SignUtils#8

Closed
Federico2014 wants to merge 9 commits intodevelopfrom
refactor/crypto/cleanup-dead-code
Closed

refactor(crypto): remove deprecated methods and harden SM2/SignUtils#8
Federico2014 wants to merge 9 commits intodevelopfrom
refactor/crypto/cleanup-dead-code

Conversation

@Federico2014
Copy link
Copy Markdown
Owner

@Federico2014 Federico2014 commented Apr 8, 2026

Summary

  • Remove compressPoint/decompressPoint from both ECKey and SM2
    (all 4 were @deprecated, zero callers confirmed by grep)
  • Add private static final to SM2 curve domain parameters
    (SM2_N, SM2_P, SM2_A, SM2_B, SM2_GX, SM2_GY)
  • Add instanceof type guards in SignUtils.signatureToAddress
    to replace silent ClassCastException with descriptive
    IllegalArgumentException on engine/signature-type mismatch

Compatibility

Removing compressPoint/decompressPoint is a binary-incompatible
change. External code calling these methods via direct reference will
encounter NoSuchMethodError at runtime after upgrading. Migration:
use ECPoint.getEncoded(boolean) directly.

Test plan

  • ./gradlew :framework:test --tests "org.tron.common.crypto.*" — all pass
  • ./gradlew :framework:test --tests "org.tron.core.net.messagehandler.PbftMsgHandlerTest" — pass
  • ./gradlew :framework:test --tests "org.tron.core.pbft.PbftApiTest" — pass
  • ./gradlew :chainbase:test — all pass

Summary by cubic

Removed deprecated point-compression helpers from ECKey and SM2, tightened SM2 constants, and added safer signature type checks in SignUtils. Also added PR coverage gates and auto reviewer assignment in CI.

  • Refactors

    • Removed compressPoint/decompressPoint from ECKey and SM2 (use ECPoint.getEncoded(boolean) instead).
    • Hardened SignUtils.signatureToAddress with instanceof guards and clear IllegalArgumentExceptions.
    • Clarified GenesisBlock timestamp validation and error messages.
  • CI

    • Added JaCoCo reports and a coverage gate: changed files >60% and overall drop ≤0.1%.
    • Introduced auto reviewer assignment by PR title scope; made PR cancel workflow more robust.
    • Removed Codecov badge; marked codecov.yml as deprecated.

Written for commit c8adb6c. Summary will update on new commits.

Summary by CodeRabbit

  • Chores

    • Enhanced CI/CD pipeline with JaCoCo code coverage gates and automated reviewer assignment for pull requests.
    • Removed deprecated point compression/decompression utility methods from cryptographic modules.
    • Improved immutability of SM2 domain parameters.
  • Bug Fixes

    • Added runtime type validation for cryptographic signatures to prevent mismatches.
    • Enhanced genesis block timestamp parsing with null-safety and range validation.
  • Documentation

    • Restructured CONTRIBUTING.md with clearer issue reporting and step-by-step contribution guidance.
    • Updated coverage tracking to use JaCoCo instead of Codecov.

@Federico2014
Copy link
Copy Markdown
Owner Author

Closing to recreate.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 8, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: caeec77a-5eeb-4c0e-873d-96660a222d9a

📥 Commits

Reviewing files that changed from the base of the PR and between bb8b4be and c8adb6c.

📒 Files selected for processing (10)
  • .github/workflows/pr-build.yml
  • .github/workflows/pr-cancel.yml
  • .github/workflows/pr-reviewer.yml
  • CONTRIBUTING.md
  • README.md
  • codecov.yml
  • common/src/main/java/org/tron/common/args/GenesisBlock.java
  • crypto/src/main/java/org/tron/common/crypto/ECKey.java
  • crypto/src/main/java/org/tron/common/crypto/SignUtils.java
  • crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java

📝 Walkthrough

Walkthrough

This PR introduces Jacoco-based coverage gates into the CI/CD pipeline, restructures documentation for issue reporting, removes deprecated point compression methods from cryptographic utilities, adds runtime type validation for signature operations, and implements automatic reviewer assignment for pull requests.

Changes

Cohort / File(s) Summary
CI/CD Workflow Enhancements
.github/workflows/pr-build.yml, .github/workflows/pr-cancel.yml, .github/workflows/pr-reviewer.yml
Added Jacoco coverage generation and a multi-job coverage-gate workflow that compares base vs. PR coverage with configurable thresholds (overall ≥ -0.1%, changed-files > 60%). Fixed null-pointer guard in pr-cancel.yml. Introduced new pr-reviewer.yml workflow to auto-assign reviewers based on conventional commit scopes in PR titles.
Documentation & Configuration Updates
CONTRIBUTING.md, README.md, codecov.yml
Restructured "Reporting An Issue" section with separate subsections for questions, bugs, and features. Reorganized "Submitting Code" contribution steps. Removed Codecov badge from README. Added deprecation notice to codecov.yml indicating JaCoCo handles coverage.
Genesis Block Timestamp Validation
common/src/main/java/org/tron/common/args/GenesisBlock.java
Enhanced setTimestamp() to conditionally parse and validate the timestamp field: defaults to DEFAULT_TIMESTAMP when null; validates non-negative values and throws IllegalArgumentException on parse failures.
Cryptography API Refinement
crypto/src/main/java/org/tron/common/crypto/ECKey.java, crypto/src/main/java/org/tron/common/crypto/SignUtils.java, crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java
Removed deprecated public point compression/decompression methods from ECKey and SM2 classes. Made SM2 domain parameter constants immutable (final). Enhanced SignUtils to enforce runtime type compatibility: validates signatureInterface matches the selected crypto engine (ECDSASignature for EC, SM2Signature for SM2).

Sequence Diagram(s)

sequenceDiagram
    participant PR as Pull Request
    participant Build as Build & Test (PR)
    participant Coverage as Jacoco Report (PR)
    participant Artifact as Artifact Storage
    participant Base as Build & Test (Base)
    participant CoverageBase as Jacoco Report (Base)
    participant Gate as Coverage Gate
    participant Report as Step Summary

    PR->>Build: Trigger build job
    Build->>Coverage: Generate jacocoTestReport.xml
    Coverage->>Artifact: Upload jacoco-coverage-pr artifact
    
    PR->>Base: Trigger coverage-base job
    Base->>CoverageBase: Generate jacocoTestReport.xml
    CoverageBase->>Artifact: Upload jacoco-coverage-base artifact
    
    Artifact->>Gate: Download both artifacts
    Gate->>Gate: Parse & aggregate coverage reports
    Gate->>Gate: Compute delta (PR - Base)
    Gate->>Gate: Evaluate thresholds<br/>(overall ≥ -0.1%, changed-files > 60%)
    Gate->>Report: Write summary & status
    
    alt Thresholds Met
        Gate-->>PR: ✓ Pass
    else Thresholds Failed
        Gate-->>PR: ✗ Fail workflow
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

  • Removal of deprecated point compression/decompression methods from ECKey and SM2 aligns with broader effort to clean up unused SM2 cryptographic code and simplify the crypto API surface.

Poem

🐰 Coverage gates now guard the code,
With Jacoco's watchful mode.
Reviewers assigned with scope-aware care,
Deprecated methods vanish in air! ✨

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/crypto/cleanup-dead-code

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants