fix(client): stop signing on GetRawData failure; keep RecoverPubkey pure

[fbsobreira]

Summary

Closes remediation roadmap PR-6 (findings W50, W55): signing-path integrity bugs that produced wrong signatures or mutated caller data without errors.

• W50: hardwareSignTxForSending no longer discards GetRawData errors (which previously allowed ledger.SignTx on a nil/empty payload). txConfirmation wraps hash failures with %w instead of replacing the cause. GetRawData returns a clear error for a nil transaction.
• W55: RecoverPubkey copies the 65-byte signature before Ethereum-style V normalization so callers can re-verify, serialize, or broadcast the original bytes.

Test plan

• go test ./pkg/client/transaction/ ./pkg/keystore/ -count=1
• New coverage: nil-tx → hardware-sign failure; hash-error wrapping via errors.Unwrap; RecoverPubkey non-mutation with high V
• CI green on this PR

Summary by CodeRabbit

• Bug Fixes

  • Transaction signing now stops safely when raw transaction data is invalid and provides clearer error details.
  • Transaction hash and confirmation errors now preserve their underlying causes.
  • Public-key recovery no longer modifies the original signature data provided by callers.

• Tests

  • Added regression coverage for signing, transaction errors, and signature handling.

[coderabbitai]

📝 Walkthrough

Walkthrough

Transaction signing now validates raw data and preserves raw-data and transaction-hash errors with context. Nil and empty transaction data return errors. Public-key recovery copies signatures before normalization. Regression tests cover these behaviors.

Changes

Transaction integrity

Layer / File(s)	Summary
Transaction error propagation pkg/client/transaction/controller.go, pkg/client/transaction/controller_test.go	GetRawData rejects nil transactions, missing raw data, and empty serialized payloads. Ledger signing stops when raw-data retrieval fails. Transaction-hash errors preserve their causes. Tests cover signing and confirmation paths.
Signature input preservation pkg/keystore/recover.go, pkg/keystore/crypto_internal_test.go	RecoverPubkey normalizes an internal signature copy. Tests verify that the caller-provided signature remains unchanged.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• fbsobreira/gotron-sdk#278: Both changes modify transaction hashing, signing, error propagation, and confirmation behavior in pkg/client/transaction/controller.go.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the two main fixes: preventing signing after GetRawData failure and preserving RecoverPubkey purity.

✨ 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 fix/signing-path-integrity

---

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

[coderabbitai]

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 `@pkg/client/transaction/controller.go`:
- Around line 115-121: Update GetRawData and the ledger-signing path around
hardwareSignTxForSending to reject transactions whose serialized raw data is nil
or zero-length, including &core.Transaction{}. Preserve the existing error
propagation and prevent ledger.SignTx from receiving an empty payload. Add
regression coverage for GetRawData and Ledger signing with an empty transaction.

🪄 Autofix

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.yaml

Review profile: CHILL

Plan: Pro

Run ID: 52ec9194-3ba7-4272-abd2-ed6bde912ecf

📥 Commits

Reviewing files that changed from the base of the PR and between 3a24b60 and 7cdb9aa.

📒 Files selected for processing (4)

• pkg/client/transaction/controller.go
• pkg/client/transaction/controller_test.go
• pkg/keystore/crypto_internal_test.go
• pkg/keystore/recover.go

[codecov]

Codecov Report

❌ Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.92%. Comparing base (3a24b60) to head (ea45e65).

Files with missing lines	Patch %	Lines
pkg/client/transaction/controller.go	87.50%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #298      +/-   ##
==========================================
+ Coverage   82.77%   82.92%   +0.15%     
==========================================
  Files          77       77              
  Lines        6333     6348      +15     
==========================================
+ Hits         5242     5264      +22     
+ Misses        785      779       -6     
+ Partials      306      305       -1     

Files with missing lines	Coverage Δ
pkg/keystore/recover.go	86.66% <100.00%> (+2.05%)	⬆️
pkg/client/transaction/controller.go	85.58% <87.50%> (+9.05%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

🧹 Nitpick comments (1)

pkg/client/transaction/controller_test.go (1)

425-443: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use a table-driven test for invalid raw-data cases.

These tests repeat the setup and assertions. Combine them into one table-driven test with the transaction fixture and expected error as fields. This follows the repository rule to use table-driven tests.

🤖 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 `@pkg/client/transaction/controller_test.go` around lines 425 - 443, The
separate TestGetRawData_NilRawData and TestGetRawData_EmptyRawData tests
duplicate setup and assertions; combine them into one table-driven test with
cases containing the transaction fixture and expected error substring. Iterate
over the cases with subtests, preserving the existing error and nil-raw
assertions for both invalid RawData scenarios.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@pkg/client/transaction/controller_test.go`:
- Around line 425-443: The separate TestGetRawData_NilRawData and
TestGetRawData_EmptyRawData tests duplicate setup and assertions; combine them
into one table-driven test with cases containing the transaction fixture and
expected error substring. Iterate over the cases with subtests, preserving the
existing error and nil-raw assertions for both invalid RawData scenarios.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 59df1a00-52cc-4581-8b4c-64eeb2af8d06

📥 Commits

Reviewing files that changed from the base of the PR and between 7cdb9aa and ea45e65.

📒 Files selected for processing (2)

• pkg/client/transaction/controller.go
• pkg/client/transaction/controller_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/client/transaction/controller.go