Skip to content

Better reporting of negative balance in transaction balancing#799

Merged
carbolymer merged 1 commit into
masterfrom
mgalazyn/fix/better-reporting-of-negative-balance-in-autobalancing
Apr 14, 2025
Merged

Better reporting of negative balance in transaction balancing#799
carbolymer merged 1 commit into
masterfrom
mgalazyn/fix/better-reporting-of-negative-balance-in-autobalancing

Conversation

@carbolymer
Copy link
Copy Markdown
Contributor

@carbolymer carbolymer commented Apr 3, 2025 •
edited
Loading

Changelog

- description: |
    Better reporting of negative balance in transaction balancing. Remove redundant `Either` from `evaluateTransactionExecutionUnits` and `evaluateTransactionExecutionUnitsShelley` signatures.

# uncomment types applicable to the change:
  type:
  # - feature        # introduces a new feature
   - breaking       # the API has changed in a breaking way
  # - compatible     # the API has changed but is non-breaking
  # - optimisation   # measurable performance improvements
   - refactoring    # QoL changes
   - bugfix         # fixes a defect
  # - test           # fixes/modifies tests
  # - maintenance    # not directly related to the code
  # - release        # related to a new release preparation
  # - documentation  # change in code docs, haddocks...

Context

Fixes: #789 . Now instead of runtime exception, TxBodyErrorBalanceNegative is returned.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@carbolymer carbolymer force-pushed the mgalazyn/fix/better-reporting-of-negative-balance-in-autobalancing branch 2 times, most recently from 2f12867 to 7f277ba Compare April 3, 2025 13:45
@carbolymer carbolymer self-assigned this Apr 3, 2025
@carbolymer carbolymer force-pushed the mgalazyn/fix/better-reporting-of-negative-balance-in-autobalancing branch from 7f277ba to 2c1c371 Compare April 4, 2025 17:14
@carbolymer carbolymer linked an issue Apr 4, 2025 that may be closed by this pull request
[BUG] - Add error reporting of not enough value in makeTransactionBodyAutoBalance when there's not enough inputs #789
Closed
@carbolymer carbolymer marked this pull request as ready for review April 4, 2025 17:17
@carbolymer carbolymer force-pushed the mgalazyn/fix/better-reporting-of-negative-balance-in-autobalancing branch from 2c1c371 to b36cb5c Compare April 4, 2025 17:23
@carbolymer
Copy link
Copy Markdown
Contributor Author

carbolymer commented Apr 4, 2025

@Unisay FYI

@carbolymer carbolymer force-pushed the mgalazyn/fix/better-reporting-of-negative-balance-in-autobalancing branch from b36cb5c to 98cb118 Compare April 11, 2025 07:28
Jimbo4350
Jimbo4350 approved these changes Apr 11, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM! A couple comments.

Comment thread cardano-api/src/Cardano/Api/Internal/Fees.hs
-> Either
(TransactionValidityError era)
(Map ScriptWitnessIndex (Either ScriptExecutionError (EvalTxExecutionUnitsLog, ExecutionUnits)))
-> Map ScriptWitnessIndex (Either ScriptExecutionError (EvalTxExecutionUnitsLog, ExecutionUnits))
Copy link
Copy Markdown
Contributor

@Jimbo4350 Jimbo4350 Apr 11, 2025

Choose a reason for hiding this comment

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

💪

Comment thread cardano-api/src/Cardano/Api/Internal/Fees.hs
TxBodyScriptBadScriptValidity
| -- | There is not enough ada to cover both the outputs and the fees.
-- The transaction should be changed to provide more input ada, or
| -- | There is not enough ada and non-ada to cover both the outputs and the fees.
Copy link
Copy Markdown
Contributor

@Jimbo4350 Jimbo4350 Apr 11, 2025

Choose a reason for hiding this comment

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

Why non-ada? Multi-asset would be the correct term but even then multi-assets do not count towards the tx fee. I think you're trying to say the transaction is unbalanced with respect to multi-assets.

Copy link
Copy Markdown
Contributor Author

@carbolymer carbolymer Apr 11, 2025

Choose a reason for hiding this comment

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

I mean ada and non-ada assets here as well. Previously this error was used for ada balancing only. Now it's returned when the multi-assets balance is negative as well.

Copy link
Copy Markdown
Contributor

@Jimbo4350 Jimbo4350 Apr 14, 2025

Choose a reason for hiding this comment

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

I was referencing the term "non-ada" itself. Substituting "non-ada" with "multi-assets" is what I was suggesting.

Comment thread cardano-api/src/Cardano/Api/Internal/Fees.hs
Comment thread cardano-api/src/Cardano/Api/Internal/Fees.hs
onlyAda = null . toList . filterValue isNotAda
balanceCheck sbe bpparams txout@(TxOut _ balance _ _) = do
let outValue@(L.MaryValue coin multiAsset) = toMaryValue $ txOutValueToValue balance
isPositiveValue = L.pointwise (>) outValue mempty
Copy link
Copy Markdown
Contributor

@Jimbo4350 Jimbo4350 Apr 11, 2025

Choose a reason for hiding this comment

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

💪

@carbolymer carbolymer force-pushed the mgalazyn/fix/better-reporting-of-negative-balance-in-autobalancing branch from 98cb118 to 857f03d Compare April 11, 2025 15:53
@carbolymer carbolymer added this pull request to the merge queue Apr 14, 2025
Merged via the queue into master with commit 09c7498 Apr 14, 2025
29 checks passed
@carbolymer carbolymer deleted the mgalazyn/fix/better-reporting-of-negative-balance-in-autobalancing branch April 14, 2025 15:58
@carbolymer carbolymer mentioned this pull request Apr 22, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts

@erikd erikd Awaiting requested review from erikd erikd is a code owner

@newhoggy newhoggy Awaiting requested review from newhoggy

@smelc smelc Awaiting requested review from smelc

@palas palas Awaiting requested review from palas palas is a code owner

@kevinhammond kevinhammond Awaiting requested review from kevinhammond

@CarlosLopezDeLara CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara CarlosLopezDeLara is a code owner

@disassembler disassembler Awaiting requested review from disassembler disassembler is a code owner

Assignees

@carbolymer carbolymer

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] - Add error reporting of not enough value in makeTransactionBodyAutoBalance when there's not enough inputs

2 participants

@carbolymer @Jimbo4350