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.

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

transaction-build and build-estimate: include current treasury value only if a donation is being done #826

Merged

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Jul 5, 2024

Changelog

- description: |
    transaction-build and build-estimate: include current treasury value only if a donation is being done
# 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 #825

How to trust this PR

  • A new test is added
  • Maybe add a negative test?

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

Comment on lines 1212 to 1216
pCurrentTreasuryValueAndDonation sbe =
caseShelleyToBabbageOrConwayEraOnwards
(const $ pure Nothing)
(const $ optional $ TxCurrentTreasuryValue <$> coinParser)
where
coinParser :: Parser L.Coin =
Opt.option (readerFromParsecParser parseLovelace) $ mconcat
[ Opt.long "current-treasury-value"
, Opt.metavar "LOVELACE"
, Opt.help "The current treasury value."
]
(const $ optional $ ((,) <$> pCurrentTreasuryValue' <*> pTreasuryDonation'))
sbe

Check warning

Code scanning / HLint

Eta reduce Warning

cardano-cli/src/Cardano/CLI/EraBased/Options/Common.hs:(1212,1)-(1216,7): Warning: Eta reduce
  
Found:
  pCurrentTreasuryValueAndDonation sbe
    = caseShelleyToBabbageOrConwayEraOnwards
        (const $ pure Nothing)
        (const
           $ optional ((,) <$> pCurrentTreasuryValue' <> pTreasuryDonation'))
        sbe
  
Perhaps:
  pCurrentTreasuryValueAndDonation
    = caseShelleyToBabbageOrConwayEraOnwards
        (const $ pure Nothing)
        (const
           $ optional ((,) <$> pCurrentTreasuryValue' <> pTreasuryDonation'))
Copy link
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.

There is a left over undefined.

let currentTreasuryValueAndDonation =
case (treasuryDonation, unFeatured <$> featuredCurrentTreasuryValueM) of
(Nothing, _) -> Nothing -- We shouldn't specify the treasury value when no donation is being done
(Just _td, Nothing) -> undefined -- TODO: Current treasury value couldn't be obtained but is required: we should fail suggesting that the node's version is too old
Copy link
Contributor

Choose a reason for hiding this comment

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

Left over undefined

@Jimbo4350 Jimbo4350 force-pushed the smelc/tx-build-no-donation-then-no-current-treasury-value branch from 7535c58 to cdc2262 Compare July 8, 2024 16:46
@Jimbo4350 Jimbo4350 self-requested a review July 8, 2024 16:46
@Jimbo4350 Jimbo4350 force-pushed the smelc/tx-build-no-donation-then-no-current-treasury-value branch from cdc2262 to 66ddd93 Compare July 8, 2024 16:50
@Jimbo4350 Jimbo4350 enabled auto-merge July 8, 2024 16:50
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Jul 8, 2024
Merged via the queue into main with commit ab1dde3 Jul 8, 2024
21 checks passed
@Jimbo4350 Jimbo4350 deleted the smelc/tx-build-no-donation-then-no-current-treasury-value branch July 8, 2024 18:03
@@ -255,7 +261,7 @@ runTransactionBuildEstimateCmd
:: ()
=> Cmd.TransactionBuildEstimateCmdArgs era
-> ExceptT TxCmdError IO ()
runTransactionBuildEstimateCmd
runTransactionBuildEstimateCmd -- TODO change type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a leftover TODO. It can be removed. cc @CarlosLopezDeLara

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build cmd is populating currentTreasuryValue on every TX. It should not.
2 participants