Skip to content

feat(protocol): add protoLint check for enum validation#6631

Open
0xbigapple wants to merge 4 commits intotronprotocol:developfrom
0xbigapple:feature/proto-lint-validation
Open

feat(protocol): add protoLint check for enum validation#6631
0xbigapple wants to merge 4 commits intotronprotocol:developfrom
0xbigapple:feature/proto-lint-validation

Conversation

@0xbigapple
Copy link
Copy Markdown
Collaborator

What does this PR do?

  • Enforce UNKNOWN_ prefix for Enum index 0 to ensure JSON serialization compatibility.
  • Included a legacyEnums Set to make sure the CI passes for old code.
  • Bound to the standard check task (check.dependsOn protoLint) which run automatically during CI and local full builds. Also support run manually with ./gradlew :protocol:protoLint or ./gradlew :protocol:check.
  • Add multi-platform buf (v1.61.0) checksums to verification-metadata.xml.

Why are these changes required?
close #6515

This PR has been tested by:

  • Unit Tests
  • Manual Testing

Follow up

Extra details

  • FAIL OUTPUT:
> Task :protocol:protoLint FAILED
🔍 Generating Proto AST image using buf CLI...

❌ [Protocol Design Violation] The following enums violate the java-tron API evolution standard:
  - [core/contract/common.proto] Enum "ResourceCode" has index 0: "BANDWIDTH". It MUST start with "UNKNOWN_".
  - [core/Tron.proto] Enum "Permission.PermissionType" has index 0: "Owner". It MUST start with "UNKNOWN_".

FAILURE: Build failed with an exception.
  • SUCCESS OUTPUT:
> Task :protocol:protoLint
🔍 Generating Proto AST image using buf CLI...
✅ Proto Enum validation passed!

@vividctrlalt
Copy link
Copy Markdown
Contributor

Nice work — this solves a real problem and the implementation is solid. A few suggestions:

Should fix:

  1. org.gradle.internal.os.OperatingSystem (protoLint.gradle:15) — This is a Gradle internal API with no stability guarantee across major versions. The root build.gradle already uses System.getProperty("os.name") for platform detection, which is a stable public API. Suggest using the same approach here.

  2. buf stderr not captured (protoLint.gradle:91-98) — When buf fails, the error message only shows the exit code. All diagnostic output (proto syntax errors, compilation failures) is lost. Adding standardOutput/errorOutput capture to the exec block would save developers from having to rerun buf manually to debug.

Worth considering:

  1. Incremental build inputs incomplete (protoLint.gradle:43-44) — inputs doesn't include the buf version or build/extracted-include-protos/ directory. Upgrading buf or changing external proto dependencies may not invalidate the task cache. Adding inputs.property('bufVersion', bufVersion) would fix the version case.

  2. Legacy whitelist maintenance — The 21-entry whitelist is hardcoded with no documented update process. A new contributor whose enum fails validation won't know this list exists. A comment pointing to the issue or a note in CONTRIBUTING.md would help.

  3. Enum vs message detection (protoLint.gradle:133) — definition.value != null as the discriminator works but is undocumented. A brief comment explaining why this is correct in buf's JSON AST format would help future maintainers.

  4. check.dependsOn protoLint (protoLint.gradle:167) — This runs proto validation on every gradle build, including local dev builds that don't touch proto files. Consider onlyIf { providers.environmentVariable('CI').present } or scoping it to proto-change detection so local iteration stays fast.

  5. v1beta1 config format (protoLint.gradle:89) — The inline buf config uses a beta format. A static buf.yaml file in the protocol/ directory would be more forward-compatible and auto-detected by buf.

@0xbigapple
Copy link
Copy Markdown
Collaborator Author

@vividctrlalt Thanks for the detailed review! I really appreciate the feedback and the time you took to look into this. I've considered the points you raised and would like to share my thoughts for discussion:

Regarding the "Should fix" items:

  1. OperatingSystem: I understand the concern about using a Gradle internal API. However, org.gradle.internal.os.OperatingSystem is already being used in the root build.gradle and framework/build.gradle. I leaned towards keeping it here to maintain a consistent approach to platform detection throughout the project.
  2. buf stderr not captured: By default, Gradle's exec block pipes all stdout and stderr directly to the console, so any diagnostic details (like syntax errors) from buf are actually visible in the build logs. The reason I set ignoreExitValue = true was to prevent Gradle from immediately throwing its generic "non-zero exit value" error. This allows the script to catch the failure and throw a more descriptive GradleException with a helpful hint ("Ensure your .proto files are valid"), while still preserving the full buf output in the console.

Regarding the "Worth considering" items:

  1. Incremental build inputs: Since bufVersion is defined directly inside protoLint.gradle, and the script itself is registered as an input (inputs.file('protoLint.gradle')), any change to the version will modify the script and correctly invalidate the cache. As for external protos, since this linting is strictly focused on our internal java-tron enum standards, not triggering a rerun on external dependency changes seems like the right boundary.
  2. Legacy whitelist maintenance: I've included a link to the original design issue (AccountCapsule and Wallet` frozenV2 construction logic #6515) in the header comments at the top of the file. Hopefully, this provides the necessary historical context for any new contributor who needs to understand the standard.
  3. Enum vs message detection: Great suggestion! I have added the comments in the lasted commit.
  4. check.dependsOn protoLint: Thanks to the inputs and outputs configuration, Gradle's incremental build (UP-TO-DATE) feature will skip this task entirely if no proto files or the script itself have been modified. Therefore, it shouldn't add any overhead to everyday local dev builds.
  5. v1beta1 config format: I agree that a static buf.yaml is the standard approach. I kept the Buf config inline to avoid introducing an extra config file for a narrowly scoped rule. Maybe this should be revisited if we later upgrade Buf or standardize Buf usage more broadly in the repository.

I hope these explanations clarify the design choices! Let me know what you think about these points, and I'm happy to discuss further if you feel strongly about any of them.

@vividctrlalt
Copy link
Copy Markdown
Contributor

Thanks for the thorough responses — your points on #2 (stderr default passthrough) and #3 (script-as-input covers version changes) are correct. I agree with all your explanations.

Two suggestions going forward:

  1. Internal API (org.gradle.internal.os.OperatingSystem) — Keeping it consistent with the rest of the project makes sense for now. Suggest a separate PR in the future to unify platform detection across all Gradle scripts to public APIs, reducing risk if Gradle drops this internal class in a future major version.

  2. Extract legacy enum whitelist to a separate file — The 21-entry whitelist in the Gradle script works, but as it grows or changes, a dedicated file (e.g. protocol/legacy-enums.txt) would be easier to maintain and review. Could be done in this PR if straightforward, otherwise as a follow-up.

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

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

AccountCapsule and Wallet` frozenV2 construction logic

4 participants