Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Prepare for rewriting balanceTransaction #3150

Merged
merged 17 commits into from
Feb 25, 2022
Merged

Prepare for rewriting balanceTransaction #3150

merged 17 commits into from
Feb 25, 2022

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Feb 22, 2022 •
edited
Loading

Comments

Preparation for #3100

Issue Number

ADP-1372

@Anviking Anviking self-assigned this Feb 22, 2022
@Anviking Anviking force-pushed the anviking/ADP-1372/improve-tests branch 2 times, most recently from e3e4962 to 37f8a0c Compare February 22, 2022 18:12
Anviking
Anviking commented Feb 22, 2022
View reviewed changes
lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs Outdated
Comment on lines 622 to 626
errBadEra = error $ unwords
[ "evaluateTransactionBalance:"
, "cannot add a datum hash to the transaction body of an"
, "era which doesn't support datum hashes."
]
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I'd address these, but TBH, because there probably are higher priority problems, it might actually make sense to leave these for now...

@Anviking
Copy link
Member Author

bors try

iohk-bors bot added a commit that referenced this pull request Feb 22, 2022
@iohk-bors
Try #3150:
0cf1909
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 22, 2022

try

Build succeeded:

jonathanknowles
jonathanknowles reviewed Feb 24, 2022
View reviewed changes
Copy link
Contributor

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

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

Hi @Anviking

Thanks for making this PR!

I don't fully understand it yet, but I've left some comments and small requests.

Hopefully we can have a call to help clear up any misunderstandings that I have.

Thanks!

lib/core/src/Cardano/Wallet.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/Api/Server.hs Outdated
[ "What was supposed to be an initial overestimation of fees "
, "turned out to be an underestimation, and I cannot recover. "
, "This is a cardano-wallet bug."
[ "I have somehow failed balancing the transaction. The balance"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a suggestion:

Suggested change
[ "I have somehow failed balancing the transaction. The balance"
[ "I have failed to balance the transaction. The balance"

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 I do like the "somehow" however, to signal that this is unexpected -> f347a64

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! Perhaps it's good to include the "somehow" after all. I agree it's nice to signal that this is unexpected.

lib/core/src/Cardano/Wallet/Transaction.hs
-> Maybe Node.Value
-- ^ Evaluate the balance of a transaction using the ledger. A valid
-- transaction must be balanced.
--
Copy link
Contributor

@jonathanknowles jonathanknowles Feb 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this usage of the term "balance" might be confusing for people.

There is a pre-existing convention that "balance" refers to the sum of something. For example:

  • The "available UTxO balance": the sum of all values in the wallet's UTxO set.
  • The "input balance": the sum of all values provided by inputs in a transaction.
  • The "output balance": the sum of all values consumed by outputs in a transaction.

I think this usage of "balance" as the "sum" of something is quite intuitive.

Though in "evaluateTransactionBalance", "balance" seems to refer to a delta between input value provided to the transaction and output value consumed by the transaction. In other words, it's a delta between two sums: Σ input value − Σ output value.

Could we consider the term "delta" instead? This usage already has a precedent: https://github.com/input-output-hk/cardano-wallet/blob/bfacc71ada6b926ee608e5e537e7040d37842eaf/lib/core/src/Cardano/Wallet/CoinSelection/Internal/Balance.hs#L510

Alternatively, if you're really keen on using the term "balance" here, then perhaps we should add some more comments to define what sort of "balance" it is. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

As a further suggestion: if we adopt the term "balance", then perhaps we could add a comment similar to:

The balance of a transaction is defined as:
(value consumed by the transaction) − (value produced by the transaction)

This way, we avoid having to mention the fee, since the fee is part of the value that's "consumed" by the transaction.

Copy link
Member Author

Choose a reason for hiding this comment

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

f20fa22

I'm in favour of balance as a term because it seems well-used in the node and ledger — including the ledger's evaluateTransactionBalance we make use of.

Delta seems to be a concept specific to coin-selection, and differs from balance in that it is not affected by the fee.

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs Outdated Show resolved Hide resolved
lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs Outdated Show resolved Hide resolved
lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs Show resolved Hide resolved
lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs Outdated Show resolved Hide resolved
lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs Outdated
instance Buildable BalanceTxGolden where
build (BalanceTxGoldenFailure c err) = mconcat
[ build c
, ",,,"
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: why three commas here? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I know, it looks strange! The idea was to have well-defined CSV columns for the sake of gnuplot. However, leaving empty values didn't seem to work... Maybe I should use a single comma unless we come up with a proper checked-in gnuplot script.

Copy link
Member Author

Choose a reason for hiding this comment

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

-> ece4adc, not sure what you think.

On a related note: having a concise ToText for balanceTx/coin-selection errors would be very neat!

Copy link
Contributor

Choose a reason for hiding this comment

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

On a related note: having a concise ToText for balanceTx/coin-selection errors would be very neat!

Perhaps rendering these as JSON makes more sense? Since these errors sometimes have to contain structured information.

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs Outdated Show resolved Hide resolved
lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs Outdated Show resolved Hide resolved
@Anviking Anviking force-pushed the anviking/ADP-1372/improve-tests branch 5 times, most recently from d948111 to 57bc6fa Compare February 24, 2022 14:40
@jonathanknowles jonathanknowles self-requested a review February 25, 2022 10:12
jonathanknowles
jonathanknowles approved these changes Feb 25, 2022
View reviewed changes
Copy link
Contributor

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

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

Hi @Anviking

I've added a few comments to add links to tickets on JIRA for the various TODO comments. If you could add the links, and link those JIRA tickets to the appropriate epic, then I think it would be fine to merge!

I've also added a few other comments and suggestions. Hopefully these should be make sense, but let me know if not.

Thanks again!

Jonathan

@Anviking
Add evaluateTransactionBalance to TransactionLayer
b8d746e 
Squashed:

Tweak arguments of evaluateTransactionBalance

Allow passing in the UTxO in two parts:
1. `W.UTxO`
2. `[(TxIn, TxOut, Maybe (Hash "Datum"))]`

which are internally combined.

This could have been avoided if either:
1. `W.UTxO` could support datum hashes
2. A `UTxO -> Cardano.UTxO` conversion were available in core.

Improve errors of evaluateTransactionBalance
@Anviking
Provide more details in UnderestimatedFee
d378250
@Anviking
Guard against returning imbalanced transactions with err500
74477f5
Anviking and others added 13 commits February 25, 2022 15:28
@Anviking
Replace MonadRandom m with StdGenSeed in tests
ba6145c
@Anviking
Improve mock ProtocolParameters
ef0d4eb
@Anviking
Add balanceTransaction goldens
fceb1d9 
Produces CSV files with the columns
> wallet balance, fee, minfee, error
for a 1 UTxO wallet balance of 0 to 4 ada,

with the following payloads:
- 1ada-payment
- delegate
- pingPong_1
- pingPong_2
@Anviking
Clarify reason for "Extra UTxO" param
3e91108
@Anviking @jonathanknowles
Address review suggestions + rebase fixups
3f0df76 
Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
@Anviking
Regenerate golden
28ce872
@Anviking
Adjust coverage condition of prop_balanceTransactionUnresolvedInputs
4a17f50
@Anviking
Remove superflous commas in balanceTx golden format
5591109
@Anviking
Reword ErrBalanceTxFailedBalancing
48f5c32
@Anviking
Improve evaluateTransactionBalance documentation
f7b3596
@Anviking
Clarify mockProtocolParametersForBalancing
bf74d3a
@Anviking
Remove unneeded partial record selectors
38e3d9a
@Anviking
Address review suggestions (better docs + formatting)
2453aa1
@Anviking Anviking force-pushed the anviking/ADP-1372/improve-tests branch from 253402e to 2453aa1 Compare February 25, 2022 14:30
Anviking
Anviking commented Feb 25, 2022
View reviewed changes
lib/shelley/src/Cardano/Wallet/Shelley/Compatibility.hs
@@ -1722,6 +1724,13 @@ toCardanoStakeCredential = Cardano.StakeCredentialByKey
toCardanoLovelace :: W.Coin -> Cardano.Lovelace
toCardanoLovelace (W.Coin c) = Cardano.Lovelace $ intCast c

toCardanoUTxO :: ShelleyBasedEra era -> W.UTxO -> Cardano.UTxO era
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this will only be needed from the tests, but keeping for now...

@Anviking
Copy link
Member Author

Thanks @jonathanknowles !

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 25, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit 32b3915 into master Feb 25, 2022
@iohk-bors iohk-bors bot deleted the anviking/ADP-1372/improve-tests branch February 25, 2022 17:36
WilliamKingNoel-Bot pushed a commit that referenced this pull request Feb 25, 2022
docs: Merge #3150
3790aad
@Anviking Anviking mentioned this pull request Feb 25, 2022
Merged
7 tasks
iohk-bors bot added a commit that referenced this pull request Mar 23, 2022
@iohk-bors @Anviking @jonathanknowles
Merge #3100
3bf427b 
3100: Re-write balanceTransaction using Node.evaluateTransactionBalance r=jonathanknowles a=Anviking

- [x] Rewrite balanceTransaction using `evaluateTransactionBalance`
     - Effects on goldens can be viewed in the diff of this PR
- [x] Enable `prop_balanceTransactionBalanced`
- [x] Polish
- [x] Even more polish
    - [ ] Will look through from top to bottom before merging

### Future work
- [x] Pay surplus as excess fee when we cannot afford a change-output. (Would like to do while comparing constructTx with balanceTx)
- [x] Ensure tx size limit is respected (Requires fixing ADP-1484 _and_ [this](https://github.com/input-output-hk/cardano-wallet/blob/ae8a2206b6f659b879d3bf14fd4ffdad7e781992/lib/core/src/Cardano/Wallet.hs#L1788-L1818) )
 
### Comments

First part introducing goldens and the evaluateTransactionBalance function: #3150

### Issue Number


ADP-1372

<!-- Reference the Jira/GitHub issue that this PR relates to, and which requirements it tackles.
  Note: Jira issues of the form ADP- will be auto-linked. -->


Co-authored-by: Johannes Lund <johannes.lund@iohk.io>
Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathanknowles jonathanknowles jonathanknowles approved these changes

Assignees

@Anviking Anviking

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Anviking @jonathanknowles