refactor: simplify TronTransaction raw transaction handling and remov… #126
Conversation
WalkthroughThe pull request introduces modifications to the Tron transaction handling in the codebase. The primary changes are in the Changes
Possibly Related PRs
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/src/crypto/tron/entities/tron_transaction.dart(1 hunks)lib/src/domain/entities/generic_transaction.dart(0 hunks)
💤 Files with no reviewable changes (1)
- lib/src/domain/entities/generic_transaction.dart
| @@ -140,8 +140,7 @@ base class TronTransaction extends EVMTransaction { | |||
| ); | |||
|
|
|||
| final rawData = raw_data_hex.hexToBytes; | |||
| final tron.Transaction_raw rawTx = | |||
| tron.Transaction_raw.fromBuffer(rawData); | |||
| final Transaction_raw rawTx = Transaction_raw.fromBuffer(rawData); | |||
There was a problem hiding this comment.
Add a null or empty-check for rawTx.contract to prevent runtime errors.
If rawTx.contract is empty, calling rawTx.contract.first will cause an out-of-range error. Consider adding a guard clause before accessing .first.
Proposed fix:
if (json
case {
"txID": String hash,
"net_usage": int _,
"net_fee": int netFee,
"energy_usage": int _,
"energy_fee": int energyFee,
"blockNumber": int block,
"block_timestamp": int block_timestamp,
"raw_data_hex": String raw_data_hex,
}) {
final fee = Amount(
value: BigInt.from(netFee + energyFee),
decimals: 6,
);
final rawData = raw_data_hex.hexToBytes;
+ // Guard check for empty contract list
+ if (rawData.isEmpty) {
+ return null; // or handle appropriately
+ }
- final Transaction_raw rawTx = Transaction_raw.fromBuffer(rawData);
+ final rawTx = Transaction_raw.fromBuffer(rawData);
+ if (rawTx.contract.isEmpty) {
+ return null; // or handle appropriately
+ }
final contract = rawTx.contract.first;
...
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/dart.yml (2)
57-57: LGTM! Remove trailing space.The condition to restrict documentation generation to the main branch is a good security practice. It prevents unintended documentation updates from pull requests.
- if: github.ref == 'refs/heads/main' + if: github.ref == 'refs/heads/main'🧰 Tools
🪛 yamllint (1.35.1)
[error] 57-57: trailing spaces
(trailing-spaces)
Line range hint
19-99: Consider optimizing the workflow structure.The workflow could benefit from the following improvements:
- Extract common setup steps (Dart SDK, dependencies) into a reusable composite action
- Consider using GitHub Actions artifacts to pass documentation between jobs
- Add a dependency between jobs to ensure linting passes before documentation generation
Example optimization:
jobs: setup: runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 - name: Setup Dart SDK uses: dart-lang/setup-dart@v1 with: sdk: 'stable' - name: Get dependencies run: | dart pub get cd packages/bip39 && dart pub get - name: Cache Dart setup uses: actions/cache@v3 with: path: | .dart_tool packages/bip39/.dart_tool key: ${{ runner.os }}-dart-${{ hashFiles('**/pubspec.lock') }} lint: needs: setup # ... rest of lint job generate-docs: needs: [setup, lint] if: github.ref == 'refs/heads/main' # ... rest of generate-docs job🧰 Tools
🪛 yamllint (1.35.1)
[error] 57-57: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/dart.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/dart.yml
[error] 57-57: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/src/crypto/utxo/repositories/electrum_json_rpc_client.dart(1 hunks)lib/src/crypto/utxo/utils/send.dart(1 hunks)lib/src/crypto/utxo/utxo_analyzer.dart(1 hunks)test/no_ci/input_simulation_test.dart(2 hunks)
🔇 Additional comments (4)
lib/src/crypto/utxo/repositories/electrum_json_rpc_client.dart (1)
156-163: LGTM! Good simplification of output handling.
The changes improve code clarity by using .nonNulls and directly passing the outputs list, while maintaining the same functionality.
test/no_ci/input_simulation_test.dart (1)
206-206: LGTM! Enhanced error handling for electrum node operations.
The addition of error capture and assertion improves the robustness of the transaction fetching process.
Also applies to: 224-225
lib/src/crypto/utxo/utils/send.dart (2)
294-294: LGTM! More descriptive error message for signing failures.
The enhanced error message now includes output and input details, which will help with debugging when signing fails due to a missing node.
Line range hint 156-163: Verify the impact of output handling changes.
Let's verify that the simplified output handling doesn't affect other parts of the codebase.
✅ Verification successful
Let me gather more context about the usage of this function to ensure a thorough verification.
Let me check one more aspect to verify the output handling changes.
Output handling changes are localized and safe
The verification shows that:
- The assertion logic for total input/output value matching is unique to this file
- The
getTransactionForSimulationusage in tests and repository is independent of these output handling changes - No other parts of the codebase rely on or replicate this specific output validation pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of getTransactionForSimulation to ensure compatibility
rg -l "getTransactionForSimulation" --type dart
# Search for patterns that might be affected by the .nonNulls change
ast-grep --pattern 'getTransactionForSimulation($$$).then($$$outputs$$$)'
Length of output: 224
Script:
#!/bin/bash
# Check the actual implementation and usage in the test file
rg "getTransactionForSimulation" test/no_ci/input_simulation_test.dart -B 2 -A 2
# Check the implementation in the repository file
rg "getTransactionForSimulation" lib/src/crypto/utxo/repositories/electrum_json_rpc_client.dart -B 2 -A 2
# Look for related output handling patterns
ast-grep --pattern 'buildOutputs($$$)'
Length of output: 499
Script:
#!/bin/bash
# Check for any direct references to totalInputValue and output handling
rg "totalInputValue.*==.*targetValue.*changeValue.*estimatedFee" --type dart -B 2 -A 2
# Look for similar assertion patterns in the codebase
rg "assert.*Total.*Input.*Value.*match.*Output.*Value" --type dart -B 2 -A 2
# Check for any other similar output building patterns
rg "buildOutputs.*targetValue.*changeValue" --type dart -B 2 -A 2
Length of output: 555
| print( | ||
| "Selected ${endpoints.map( | ||
| (e) => "$e", | ||
| )}", | ||
| ); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace print statement with Logger utility
The code already imports and uses a Logger utility throughout the file (e.g., Logger.log, Logger.logFetch). For consistency and better control over logging, consider using the Logger utility instead of print.
- print(
- "Selected ${endpoints.map(
- (e) => "$e",
- )}",
- );
+ Logger.log(
+ "Selected ${endpoints.map(
+ (e) => "$e",
+ )}",
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print( | |
| "Selected ${endpoints.map( | |
| (e) => "$e", | |
| )}", | |
| ); | |
| Logger.log( | |
| "Selected ${endpoints.map( | |
| (e) => "$e", | |
| )}", | |
| ); |
…e unused import
Summary by CodeRabbit
New Features
Bug Fixes
Chores