Skip to content

fix: validate tron address checksum#577

Merged
jackz1024 merged 6 commits into
openmetaearth:mainfrom
NikYak228:me-hub-173-tron-address-validation
Jun 24, 2026
Merged

fix: validate tron address checksum#577
jackz1024 merged 6 commits into
openmetaearth:mainfrom
NikYak228:me-hub-173-tron-address-validation

Conversation

@NikYak228

@NikYak228 NikYak228 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

/claim #173

Fixes #173

Summary

  • reject Base58Check addresses whose decoded payload does not use the TRON 0x41 prefix
  • verify the decoded payload has the expected 21-byte TRON address length
  • add a regression that constructs a valid Base58Check address with a non-TRON prefix and confirms it is rejected

Tests

  • go test ./x/tron/... -count=1
  • git diff --check

Summary by CodeRabbit

  • Bug Fixes
    • Improved Tron address validation with clearer, more specific error messages for invalid address length.
    • Added explicit validation for decoded address length and the expected Tron prefix to more reliably reject malformed addresses.
  • Tests
    • Expanded coverage for invalid Tron addresses, including incorrect decoded lengths and non-Tron prefixes.
    • Updated test assertions to verify error message content rather than requiring exact full-text matches.

Copilot AI review requested due to automatic review settings June 1, 2026 15:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds stricter validation for Tron addresses by explicitly rejecting decoded Base58Check payloads with an unexpected length or a non-Tron network prefix, and extends tests to cover the non-Tron prefix case.

Changes:

  • Add decoded address length and prefix checks in ValidateTronAddress.
  • Add a test case for Base58Check addresses that have a valid checksum but non-Tron prefix.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
x/tron/types/address.go Adds length and Tron prefix validation before re-encoding and comparing.
x/tron/types/address_test.go Adds coverage for rejecting Base58Check addresses with a non-Tron prefix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread x/tron/types/address.go
Comment thread x/tron/types/address.go
Comment thread x/tron/types/address_test.go
@koelzen koelzen added run-ci Trigger CI run and removed run-ci Trigger CI run labels Jun 10, 2026
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: e8f35999-f1c9-4d40-9da6-e16f1229c4af

📥 Commits

Reviewing files that changed from the base of the PR and between 8836fcc and 0caba94.

📒 Files selected for processing (1)
  • x/tron/types/address_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • x/tron/types/address_test.go

📝 Walkthrough

Walkthrough

ValidateTronAddress now returns detailed length errors and explicitly rejects decoded addresses with the wrong byte length or Tron prefix. The test suite updates the expected error text and adds cases for non-Tron prefixes and invalid decoded lengths.

Changes

Tron Address Validation Hardening

Layer / File(s) Summary
ValidateTronAddress: length and prefix validation
x/tron/types/address.go
Replaces the generic "wrong length" error with a formatted expected/got message. Adds two explicit post-DecodeCheck validations: decoded byte length and Tron byte prefix, each returning a distinct error.
Test updates and new prefix/length cases
x/tron/types/address_test.go
Adds helpers for invalid base58check payload construction, updates the oversized-address expectation to "invalid address length", adds cases for non-Tron prefix and invalid decoded length, and switches the failing assertion to substring matching.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title mentions Tron address validation, but checksum validation is not the change; the PR adds prefix and length checks instead. Rename it to describe the actual fix, for example: "fix: reject non-TRON Base58Check addresses".
Description check ⚠️ Warning The description has a summary and tests, but it omits most required template sections like Type of Change, Checklist, and Validation Notes. Fill in the missing template sections: Type of Change, Validation Notes, Checklist, Breaking Changes, and Additional Context.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The prefix and length validation plus regression tests address the reported acceptance bug for non-TRON Base58Check addresses.
Out of Scope Changes check ✅ Passed The diff appears limited to TRON address validation and tests, with no clear unrelated changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@x/tron/types/address_test.go`:
- Around line 60-64: The base58check invalid decoded-length test in
address_test.go is too generic and can succeed on the earlier character-length
validation instead of the new decoded-length branch. Update the test case around
the address decoding checks in the relevant test function to use input that
bypasses the char-length guard and only fails at the decoded-length validation,
and tighten the assertion in the corresponding require.Contains check so it
proves the branch-specific error path is exercised.
🪄 Autofix (Beta)

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: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 680496f6-f341-4184-8764-bd3e63f9cd8d

📥 Commits

Reviewing files that changed from the base of the PR and between 77a3cc4 and c6f5bac.

📒 Files selected for processing (2)
  • x/tron/types/address.go
  • x/tron/types/address_test.go

Comment on lines +60 to +64
testName: "base58check address with invalid decoded length",
value: troncommon.EncodeCheck(shortTronPayload),
expectPass: false,
errStr: "invalid address length",
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Make the decoded-length regression case branch-specific.

Line 63 ("invalid address length") plus Line 79 (require.Contains) can pass via the earlier char-length guard, so this case does not reliably prove the new decoded-length check is exercised.

Suggested patch
@@
 		{
 			testName:   "base58check address with invalid decoded length",
 			value:      troncommon.EncodeCheck(shortTronPayload),
 			expectPass: false,
-			errStr:     "invalid address length",
+			errStr:     "expected decoded",
 		},
@@
-			require.Contains(t, err.Error(), testCase.errStr, testCase.value)
+			require.Error(t, err)
+			require.Contains(t, err.Error(), testCase.errStr, testCase.value)

As per path instructions, "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request".

Also applies to: 79-79

🤖 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 `@x/tron/types/address_test.go` around lines 60 - 64, The base58check invalid
decoded-length test in address_test.go is too generic and can succeed on the
earlier character-length validation instead of the new decoded-length branch.
Update the test case around the address decoding checks in the relevant test
function to use input that bypasses the char-length guard and only fails at the
decoded-length validation, and tighten the assertion in the corresponding
require.Contains check so it proves the branch-specific error path is exercised.

Source: Path instructions

@jackz1024 jackz1024 merged commit f177836 into openmetaearth:main Jun 24, 2026
7 checks passed
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.

[BUG BOUNTY] [Medium] [TRON] Address validation accepts non-TRON Base58Check addresses

4 participants