Reject invalid value arithmetic in pallas-validate

[LorenzoRD2003]

Summary

• use checked lovelace addition in add_values and conway_add_values
• reject negative mint/burn asset results instead of casting i64 to u64
• add regression tests for lovelace overflow and negative asset results

Closes #783

Validation

• RUSTC_WRAPPER= cargo test -p pallas-validate

Summary by CodeRabbit

• Bug Fixes
  • Improved transaction value validation to prevent arithmetic overflow and detect invalid negative values in mint operations.
  • Enhanced error handling for edge cases in multiasset coin quantity conversions.

[coderabbitai]

📝 Walkthrough

Walkthrough

Lovelace addition in add_values and conway_add_values is replaced with a new add_lovelace helper that uses checked_add, returning ValidationError on overflow. The coerce_to_coin function replaces an unchecked as u64 cast with u64::try_from, also propagating ValidationError on negative inputs. Unit tests cover all four affected code paths.

Changes

Overflow-safe value arithmetic

Layer / File(s)	Summary
Checked lovelace addition and safe coin coercion pallas-validate/src/utils.rs	Introduces private add_lovelace helper using checked_add; all Value::Coin and ConwayValue::Coin addition arms in add_values and conway_add_values route through it. coerce_to_coin replaces the unchecked *amount as u64 cast with u64::try_from(*amount) mapped to ValidationError.
Regression tests for overflow and negative-asset rejection pallas-validate/tests/utils.rs	Adds single_asset test helper and four #[test] cases: burn of a missing asset, negative-result burn, and u64::MAX + 1 overflow for both Alonzo-compatible and Conway Value types, all asserting ValidationError::PostAlonzo(PostAlonzoError::NegativeValue).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• txpipe/pallas#688: Refactors Conway multiasset addition helpers in pallas-validate/src/utils.rs using result-based propagation, overlapping with the conway_add_values code path modified here.

Suggested reviewers

• scarmuega

Poem

🐇 Hop, hop, no overflow today!
Checked_add keeps the lovelace fray at bay,
Negative casts? Try_from says "Nay!"
Errors returned in the proper way,
The ledger stays safe—hip, hip, hooray! 🌟

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Title accurately captures the main change: rejecting invalid value arithmetic through checked operations to prevent overflow and negative value handling.
Linked Issues check	✅ Passed	All coding requirements from issue #783 are met: checked lovelace addition in add_values/conway_add_values, proper i64→u64 conversion in add_minted_value, and comprehensive regression tests covering specified scenarios.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with issue #783 objectives: only modified validation functions, added necessary test coverage, no unrelated code changes detected.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pallas-validate/src/utils.rs (1)

175-183: ⚠️ Potential issue | 🔴 Critical

Fix unsafe .unwrap() in conway_coerce_to_coin.

Line 419 uses .unwrap() on PositiveCoin::try_from(*amount), causing panics on invalid multiasset quantities instead of returning a ValidationError. This contradicts the safe try_from conversion pattern used elsewhere. Replace the .unwrap() with proper error propagation:

aa.push((asset_name.clone(), PositiveCoin::try_from(*amount).map_err(|_| err.clone())?));

🤖 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 `@pallas-validate/src/utils.rs` around lines 175 - 183, In the
conway_coerce_to_coin function at line 419, replace the unsafe .unwrap() call on
PositiveCoin::try_from(*amount) with proper error propagation. Instead of
unwrapping which causes panics, use .map_err() to convert the error to a
ValidationError and propagate it with the ? operator, similar to the pattern
used elsewhere in the codebase. This ensures invalid multiasset quantities are
properly handled as validation errors rather than causing panics.

🧹 Nitpick comments (1)

pallas-validate/src/utils.rs (1)

369-379: 💤 Low value

Consider validated conversion from Coin to i64.

The cast *amount as i64 (line 374) could silently wrap to negative if an asset quantity exceeds i64::MAX. While practically unlikely in current Cardano usage, this could cause valid large values to be rejected after round-tripping through add_multiasset_values → coerce_to_coin.

This is a pre-existing concern and may be out of scope for this PR.

♻️ Suggested improvement

-            aa.push((asset_name.clone(), *amount as i64));
+            aa.push((asset_name.clone(), i64::try_from(*amount).map_err(|_| /* overflow error */)?));

🤖 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 `@pallas-validate/src/utils.rs` around lines 369 - 379, The coerce_to_i64
function performs an unchecked cast of `*amount as i64` which can silently
overflow to negative values if the asset quantity exceeds i64::MAX. Replace the
direct cast with a validated conversion approach using i64::try_from() to safely
handle the conversion from Coin to i64, and implement appropriate error handling
for cases where the value cannot fit in i64 (either by returning a Result type
or handling the overflow error case explicitly).

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

Outside diff comments:
In `@pallas-validate/src/utils.rs`:
- Around line 175-183: In the conway_coerce_to_coin function at line 419,
replace the unsafe .unwrap() call on PositiveCoin::try_from(*amount) with proper
error propagation. Instead of unwrapping which causes panics, use .map_err() to
convert the error to a ValidationError and propagate it with the ? operator,
similar to the pattern used elsewhere in the codebase. This ensures invalid
multiasset quantities are properly handled as validation errors rather than
causing panics.

---

Nitpick comments:
In `@pallas-validate/src/utils.rs`:
- Around line 369-379: The coerce_to_i64 function performs an unchecked cast of
`*amount as i64` which can silently overflow to negative values if the asset
quantity exceeds i64::MAX. Replace the direct cast with a validated conversion
approach using i64::try_from() to safely handle the conversion from Coin to i64,
and implement appropriate error handling for cases where the value cannot fit in
i64 (either by returning a Result type or handling the overflow error case
explicitly).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: cdddb354-d085-47c1-99a0-27953df2971c

📥 Commits

Reviewing files that changed from the base of the PR and between c98b5ad and 9811dd8.

📒 Files selected for processing (2)

• pallas-validate/src/utils.rs
• pallas-validate/tests/utils.rs