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

Initial lexer and parser for specifying Mary multi assets via the cli #2072

Merged
merged 1 commit into from
Nov 30, 2020

Conversation

Jimbo4350
Copy link
Contributor

@Jimbo4350 Jimbo4350 commented Nov 9, 2020

Introduce CLI parsing of a new proposed syntax for multi-asset Values and TxOuts. Note that this new syntax is backward-compatible with the old syntax. i.e. the old transaction output CLI syntax: ${address}+{lovelace} is still supported.

The new syntax is defined as follows:

Values

Specifying individual values

  • Lovelace values can be specified in the following ways:

    • ${quantity} lovelace (where quantity is a signed integer)
    • ${quantity} (where quantity is a signed integer)
  • Values for other assets can be specified in the following ways:

    • ${quantity} ${policyId}.${assetName} (where quantity is a signed integer, policyId is a hex-encoded policy ID [script hash], and assetName is an alphanumeric asset name)
    • ${quantity} ${policyId} (where quantity is a signed integer and policyId is a hex-encoded policy ID [script hash])

Negating individual values

Individual values can be negated by utilizing the - prefix operator. All individual values can be prefixed by this operator. Some examples:

  • -42 46f192e30c5d8fcc5ccbcd9c91e430283166b3382cd25261a1601b74
  • -72191 46f192e30c5d8fcc5ccbcd9c91e430283166b3382cd25261a1601b74.foo
  • -100
  • -920 lovelace

Combining individual values

Values can be combined by using the + binary operator. Some examples:

  • 42 lovelace + -1 (this would result in a Value of 41 lovelace)
  • 20 46f192e30c5d8fcc5ccbcd9c91e430283166b3382cd25261a1601b74 + 12 46f192e30c5d8fcc5ccbcd9c91e430283166b3382cd25261a1601b74.foo + -2 46f192e30c5d8fcc5ccbcd9c91e430283166b3382cd25261a1601b74.bar
  • 201 46f192e30c5d8fcc5ccbcd9c91e430283166b3382cd25261a1601b74.foo + 12 + -1 + 9 lovelace + 10 46f192e30c5d8fcc5ccbcd9c91e430283166b3382cd25261a1601b74

Transaction outputs

Transaction outputs can be specified in the following ways:

  • ${address} ${value} (where address is a Cardano address and value is a value)
  • ${address}+${value} (where address is a Cardano address and value is a value) (n.b. this is actually the old tx out CLI syntax that is additionally supported by the new parser)

@Jimbo4350 Jimbo4350 force-pushed the jordan/lexing-parsing-forge-value branch 3 times, most recently from a474787 to 71efdff Compare November 12, 2020 11:13
Copy link
Contributor

@intricate intricate left a comment

Choose a reason for hiding this comment

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

Could we expand upon this PR's description with an example(s) of the kind of input this parser is capable of handling?

cardano-cli/cardano-cli.cabal Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/test/Test/Cli/Gen.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
Copy link
Contributor

@intricate intricate left a comment

Choose a reason for hiding this comment

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

See #2072 (review).

Forgot to select "request changes" on that review.

@Jimbo4350 Jimbo4350 force-pushed the jordan/lexing-parsing-forge-value branch 3 times, most recently from af251c9 to 492e784 Compare November 18, 2020 14:41
Copy link
Contributor

@intricate intricate left a comment

Choose a reason for hiding this comment

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

Some more comments.

cardano-api/src/Cardano/Api/Value.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
@Jimbo4350 Jimbo4350 force-pushed the jordan/lexing-parsing-forge-value branch 2 times, most recently from 6ee854c to 37c5205 Compare November 26, 2020 09:25
@intricate intricate force-pushed the jordan/lexing-parsing-forge-value branch 3 times, most recently from f11528e to e275a4d Compare November 27, 2020 02:10
@intricate
Copy link
Contributor

Depends on #2129. Marking this PR as a draft until that's been merged.

@intricate intricate marked this pull request as draft November 27, 2020 02:11
@dcoutts dcoutts force-pushed the jordan/lexing-parsing-forge-value branch from e275a4d to a5a59ad Compare November 27, 2020 09:06
@dcoutts dcoutts marked this pull request as ready for review November 27, 2020 09:06
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Looking good.

I think the second patch can be dropped now, as it looks like it was applied already in another PR.

cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
@intricate intricate force-pushed the jordan/lexing-parsing-forge-value branch 2 times, most recently from 1be2c5f to 67e990b Compare November 28, 2020 22:31
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

Looking good.

cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
Comment on lines 105 to 108
data ValueExpr
= ValueExprAdd !ValueExpr !ValueExpr
| ValueExprLovelace !Quantity
| ValueExprMultiAsset !PolicyId !AssetName !Quantity
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes nice.

Comment on lines 114 to 115
tokenParser :: GenTokenParser String u Identity
tokenParser = makeTokenParser haskellDef -- TODO: What language def to use?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should avoid using this token parser. Our language is very very simple. We don't need to pull in this stuff.

We're only using it here for parens, which we don't actually want, and for reservedOp. The reservedOp is for checking that operators are not prefixes of any other reserved operators. But we have no other operators! So we can define our binary and prefix operators directly, without the token parser stuff.

cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
Comment on lines 159 to 160
period :: Parser String
period = string "."
Copy link
Contributor

Choose a reason for hiding this comment

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

If we make this have type Parser () then when we use it we don't need the _ <- syntax.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, that's true. Thanks.

cardano-cli/src/Cardano/CLI/Mary/Parser.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Typed.hs Outdated Show resolved Hide resolved
@intricate intricate self-requested a review November 30, 2020 05:43
@intricate intricate force-pushed the jordan/lexing-parsing-forge-value branch from 67e990b to 7019f0b Compare November 30, 2020 06:40
@intricate intricate force-pushed the jordan/lexing-parsing-forge-value branch from 7019f0b to 1b5494b Compare November 30, 2020 07:08
@intricate intricate self-assigned this Nov 30, 2020
@intricate intricate force-pushed the jordan/lexing-parsing-forge-value branch 2 times, most recently from 9f4bbff to dd8e1da Compare November 30, 2020 08:06
Opt.option (eitherReader readTxOut)
( Opt.long "tx-out"
<> Opt.metavar "TX-OUT"
-- TODO: Update the help text to describe the new syntax as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be addressed

@dcoutts dcoutts force-pushed the jordan/lexing-parsing-forge-value branch from 6e2299c to 24a1b77 Compare November 30, 2020 11:22
@Jimbo4350 Jimbo4350 force-pushed the jordan/lexing-parsing-forge-value branch from f669c6a to 0dd4445 Compare November 30, 2020 15:30
@dcoutts dcoutts force-pushed the jordan/lexing-parsing-forge-value branch from 0dd4445 to f669c6a Compare November 30, 2020 15:32
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

I think this is ok now. We can make further internal improvements later (e.g. the error messages).

If CI tests pass lets squash down to one patch and merge it.

@dcoutts dcoutts dismissed intricate’s stale review November 30, 2020 15:43

review now out of date

Add a concrete syntax and parser for multi-asset values. Use it in the
cli for transaction outputs and minting.

Example multi-asset values

 * "42"
   42 lovelace, same syntax as coin values in previous eras

 * "42 lovelace"
   same with the special asset id for lovelace

 * "3 e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735a.foobar"
   3 units of the asset with that policy id and asset name "foobar"

 * "3 e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735a"
   3 units of the asset with that policy id and empty asset name ""

 * "42 lovelace + 3 e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735a"
   multiple such assets can be combined with +

 * "-3 e09d36c79dec9bd1b3d9e152247701cd0bb860b5ebfd1de8abb6735a +
    42 dd0044a26cf7d4491ecea720fda11afb59d5725b53afa605fdf695e6"
   negative quantities are allowed for minting

 * "42 lovelace + -42 lovelace"
   Empty value!

Co-authored-by: Luke Nadur <19835357+intricate@users.noreply.github.com>
Co-authored-by: Duncan Coutts <duncan@well-typed.com>
@dcoutts dcoutts force-pushed the jordan/lexing-parsing-forge-value branch from 90a5f4f to db8e2f4 Compare November 30, 2020 19:01
@dcoutts
Copy link
Contributor

dcoutts commented Nov 30, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 30, 2020

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.

3 participants