Implement estimateBalancedTxBody and estimateOrCalculateBalancedTxBody #511
Conversation
Jimbo4350
requested review from
dcoutts,
erikd,
newhoggy,
carbolymer,
smelc,
palas,
kevinhammond,
CarlosLopezDeLara and
disassembler
as code owners
Jimbo4350
force-pushed
the
jordan/update-autobalance-to-handle-fee-estimation-and-calculation
branch
from
1d89840 to
1806feb
Compare
There was a problem hiding this comment.
Can you add Show and Error instances for error types?
| @@ -29,9 +29,14 @@ module Cardano.Api.Fees ( | |||
| evaluateTransactionBalance, | |||
|
|
|||
| -- * Automated transaction building | |||
| estimateBalancedTxBody, | |||
| estimateOrCalculateBalancedTxBody, | |||
There was a problem hiding this comment.
I'm thinking it would be good to have unit tests or golden tests for these new functions.
Even if it we have very basic ones to start with.
There was a problem hiding this comment.
Also what changes are we expecting in cardano-cli to follow from this PR?
There was a problem hiding this comment.
I'm thinking it would be good to have unit tests or golden tests for these new functions.
Totally agree. I have a branch open in cardano-node which is passing. I'm going to start looking at cardano-node-emulator because its annoying to have to propagate changes to cardano-testnet in order to test certain functions in cardano-api.
There was a problem hiding this comment.
Also what changes are we expecting in cardano-cli to follow from this PR?
It's not 100% clear but I would like to leave build-raw as it is and let users balance the tx on their own. I would like to modify build slightly to offer "offline" and "online" transaction balancing.
| -- Why * 100? requiredCollateral is the product of the collateral percentage and the tx fee | ||
| -- We choose to multiply 100 rather than divide by 100 to make the calculation | ||
| -- easier to manage. At the end of the calculation we then use % 100 to perform our division | ||
| -- and round up. |
There was a problem hiding this comment.
Where does the round up happen? rationalToCoinViaFloor takes the floor, so I assume that's a round down?
There was a problem hiding this comment.
So the returnCollateral is floored. This means that the requiredCollateral is rounded upwards and potentially slightly over estimated.
Jimbo4350
force-pushed
the
jordan/update-autobalance-to-handle-fee-estimation-and-calculation
branch
from
792aae4 to
0eb6628
Compare
Jimbo4350
force-pushed
the
jordan/update-autobalance-to-handle-fee-estimation-and-calculation
branch
from
0eb6628 to
fa622de
Compare
Jimbo4350
deleted the
jordan/update-autobalance-to-handle-fee-estimation-and-calculation
branch
Changelog
Context
Additional context for the PR goes here. If the PR fixes a particular issue please provide a link to the issue.
How to trust this PR
Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.
Checklist