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

Update build command to automatically calculate the total and return collateral values #4198

Merged
merged 1 commit into from
Aug 5, 2022
Merged

Update build command to automatically calculate the total and return collateral values #4198

merged 1 commit into from
Aug 5, 2022

Conversation

Jimbo4350
Copy link
Contributor

@Jimbo4350 Jimbo4350 commented Jul 19, 2022 •
edited

The build command will now automatically calculate these values and returns the excess collateral to the change address
Resolves: #4075

newhoggy
newhoggy reviewed Jul 19, 2022
View reviewed changes
cardano-api/src/Cardano/Api/Fees.hs Outdated
$ reqAmt % 100
enoughCollateral = totalCollateralLovelace * 100 >= requiredCollateral
Lovelace amt = totalCollateralLovelace * 100 - requiredCollateral
returnCollateral = fromShelleyLovelace . Ledger.rationalToCoinViaCeiling $ amt % 100
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the 100 factor for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the comments in the code

newhoggy
newhoggy reviewed Jul 19, 2022
View reviewed changes
cardano-api/src/Cardano/Api/Fees.hs
(TxReturnCollateralNone,tc@TxTotalCollateral{}) -> (dummyRetCol, tc)
(TxReturnCollateralNone, TxTotalCollateralNone) -> (dummyRetCol, dummyTotCol)

calcReturnAndTotalCollateral
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add docs and links to any supporting documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated comments in the code

@newhoggy
Copy link
Contributor

Can you paste/attach the commands/files you used to test?

This was referenced Jul 20, 2022
Open
Open
@Jimbo4350 Jimbo4350 force-pushed the jordan/send-collateral-change-to-change-addr-by-default branch from b9ac31e to 4f92308 Compare July 20, 2022 14:12
@Jimbo4350
Copy link
Contributor Author

Can you paste/attach the commands/files you used to test?

Removing the fields:

  --tx-in-collateral "$txinCollateral" \
  --tx-total-collateral 529503 \

from the shell script scripts/babbage/example-babbage-script-usage.sh is a way to test this functionality is working via a failing plutus script. We need to update the cli's tx pretty printing to include total and return collateral fields. This would be another way to inspect that the automatic balancing is working.

@CarlosLopezDeLara CarlosLopezDeLara mentioned this pull request Jul 21, 2022
Open
40 tasks
@Jimbo4350 Jimbo4350 force-pushed the jordan/send-collateral-change-to-change-addr-by-default branch 4 times, most recently from 00dbc14 to 1b9da84 Compare July 26, 2022 20:37
@saratomaz saratomaz mentioned this pull request Jul 27, 2022
Merged
saratomaz
saratomaz approved these changes Jul 27, 2022
View reviewed changes
Copy link

@saratomaz saratomaz left a comment

Choose a reason for hiding this comment

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

The PR pass in all our regression tests related to plutus V2.

Also new tests was added to comprove that the collateral amount charged are accorging to the collateral percentage present in protocol parameters and that the change address is now the default address to return the excess of the collaterals amount.

Related issue on cardano-node-tests:
IntersectMBO/cardano-node-tests#1265

@Jimbo4350 Jimbo4350 force-pushed the jordan/send-collateral-change-to-change-addr-by-default branch from 1b9da84 to 102be57 Compare July 27, 2022 15:03
@Jimbo4350
Update build command to automatically calculate the total and return
74a81ac 
collateral values when they are not explicitly specified via the cli
@Jimbo4350 Jimbo4350 force-pushed the jordan/send-collateral-change-to-change-addr-by-default branch from 102be57 to 74a81ac Compare July 27, 2022 15:07
@Jimbo4350
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 5, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit 45b0c58 into master Aug 5, 2022
@iohk-bors iohk-bors bot deleted the jordan/send-collateral-change-to-change-addr-by-default branch August 5, 2022 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@newhoggy newhoggy newhoggy left review comments

@erikd erikd erikd approved these changes

@saratomaz saratomaz saratomaz approved these changes

@dcoutts dcoutts Awaiting requested review from dcoutts

@JaredCorduan JaredCorduan Awaiting requested review from JaredCorduan

Assignees
No one assigned
Labels
None yet
Projects
No open projects
cardano-node/cardano-api/cardano-cli
Blocked
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

All balance in --tx-in-collateral is taken. Change should go to --change-address (build command)
4 participants
@Jimbo4350 @newhoggy @erikd @saratomaz