Skip to content

Remove requirement of "current treasury value" for transactions #1322

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
Open
palas wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Remove requirement of "current treasury value" for transactions #1322

palas wants to merge 7 commits into from
+173 −106

Conversation

@palas
Copy link
Contributor

@palas palas commented Jan 16, 2026 •
edited
Loading

Changelog

- description: |
    Removed requirement of "current treasury value" in transactions
  type:
  - compatible

Context

See #1149

How to trust this PR

Ensure that only the specified restriction was lifted and in the appropriate places. Ensure this is the right thing to do. Confirm I interpreted this message correctly: https://discord.com/channels/1136727663583698984/1239888777015590913/1364244737602879498

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

@palas palas self-assigned this Jan 16, 2026
@palas palas linked an issue Jan 16, 2026 that may be closed by this pull request
Remove requirement of "current treasury value" for transaction in build-raw command #1149
Open
Jimbo4350
Jimbo4350 requested changes Jan 16, 2026
View reviewed changes
@palas palas requested a review from Jimbo4350 January 17, 2026 01:12
@palas palas force-pushed the 1149-remove-requirement-of-current-treasury-withdrawal-for-transaction-in-build-raw-command branch from ca753ee to 24d7d28 Compare January 20, 2026 00:07
@palas palas force-pushed the 1149-remove-requirement-of-current-treasury-withdrawal-for-transaction-in-build-raw-command branch from d3f1e86 to edcde6f Compare January 20, 2026 15:19
Jimbo4350
Jimbo4350 requested changes Jan 26, 2026
View reviewed changes
cardano-cli/src/Cardano/CLI/EraBased/Common/Option.hs
asum
[ Opt.flag' IncludeCurrentTreasuryValue $
mconcat
[ Opt.long "include-current-treasury-value"
Copy link
Contributor

@Jimbo4350 Jimbo4350 Jan 26, 2026

Choose a reason for hiding this comment

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

Why deviate from the name that build-raw uses?

Copy link
Contributor Author

@palas palas Jan 26, 2026

Choose a reason for hiding this comment

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

The reason I used a different name for the flag is that this one obtains the value from the node, while build-raw takes it from the command line (so it expects a value after the flag). But I don't have a strong opinion about it

cardano-cli/src/Cardano/CLI/EraBased/Transaction/Command.hs

-- | Whether to include the current treasury value in the transaction body.
-- If included, the current treasury value will be obtained from the node.
data IncludeCurrentTreasuryValue = IncludeCurrentTreasuryValue | ExcludeCurrentTreasuryValue
Copy link
Contributor

@Jimbo4350 Jimbo4350 Jan 26, 2026

Choose a reason for hiding this comment

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

Why are you introducing this sum type? It's not necessary. What is wrong with (Maybe TxCurrentTreasuryValue)?

Copy link
Contributor Author

@palas palas Jan 26, 2026

Choose a reason for hiding this comment

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

It is different because that command takes the value from the node, not from the command line, so it would be equivalent to Bool.

And the reason I don't use Bool is so that the code is more strongly-typed and self-documenting. Also not a strong opinion

@palas palas requested review from Jimbo4350 and carbolymer January 27, 2026 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

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

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

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

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

Requested changes must be addressed to merge this pull request.

Assignees

@palas palas

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove requirement of "current treasury value" for transaction in build-raw command

4 participants

@palas @carbolymer @Jimbo4350