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

Add custom names pretty printing for hashes #384

Merged
merged 8 commits into from
Mar 20, 2024

Conversation

florentc
Copy link
Member

@florentc florentc commented Mar 15, 2024

This addresses #379

This PR introduces a quality of life feature to associate readable names to hashes in a new pretty-pretting option.

The option is grouped among others in the pcOptHashes field of PrettyCookedOpts. This field has the following subfields:

  • pcOptHashLength: this option already existed before, at the top level of PrettyCookedOpts, it sets how many characters of a hash to print
  • pcOptHashNames: This is the core of the new feature, it associates hashes to names. Use the hashNamesFromList smart constructor. It takes a list of key/value pairs but the keys can be anything that has a hash: wallets, scripts, minting policies, datums, transaction ids, and pretty much all the associated types (might not be exhaustive). The associated new class, Hashable, is defined in Pretty.Cooked.Hashable.
  • pcOptHashVerbose: By default, when a corresponding name is found for a hash, the hex representation is no longer printed. This flag forces it to be printed alongside the name just in case.
  • pcOptHashParseTokenNames: Not yet implemented. This is intended to parse token names for a hex representation of a hash and if something is found in pcOptHashNames, print it instead. (postponed)

Example:

pcOpts :: C.PrettyCookedOpts
pcOpts =
  def
    { C.pcOptHashes =
        def
          { C.pcOptHashNames =
              defaultHashNames
                <> C.hashNamesFromList
                  [ (alice, "Alice"),
                    (bob, "Bob"),
                    (carrie, "Carie")
                  ]
                <> C.hashNamesFromList
                  [ (nftCurrencySymbol, "NFT"),
                    (customCoinsCurrencySymbol, "Custom Coins")
                  ]
                <> C.hashNamesFromList
                  [ (fooValidator, "Foo")
                  ]
          }
    }

TODO and discussion topics

  • Remove the feature which used to display wallet numbers (they are still displayed which is useless now)
  • Maybe stop displaying "pubkey" vs "script" as a prefix when a hash has been resolved to a name (I lean towards keeping it)
  • Investigate or postpone to future PR the token name parsing feature
  • Experiment with highlighting names from the rest of the text. Hashes where easy to spot because of the # and hexadecimal characters. Now names, in particular if they have spaces or are lowercased, can be hard to notice in the middle of the text. For example Spends from script foo (Reference Script at 28282838!0), or Pays to pubkey Clint Eastwood could become Pays to pubkey [Clint Eastwood] or Spends from script [foo], Pays to pubkey #"Clint Eastwood", etc. (I'd keep it like it is for now)
  • Investigate using existential types or something to avoid having to split the names into several sublists for each concrete type. (That would be nice to have)
  • There was a hardcoded check to print "Lovelace"/"Quick"/"Permanent" for the relevant currency symbols. Should this now be moved to the default name table (which would contain those 3 elements instead of being empty by default)? (I find it would be conceptually more elegant but maybe prone to accidental overwrites in practice)

@mmontin
Copy link
Collaborator

mmontin commented Mar 19, 2024

Maybe stop displaying "pubkey" vs "script" as a prefix when a hash has been resolved to a name (I lean towards keeping it)

I lean towards keeping it either. It does not hurt and bring an info that somebody who has not worked on a codebase will need in order to understand the outcome of a trace.

Investigate or postpone to future PR the token name parsing

I believe it should be postponed.

Experiment with highlighting names from the rest of the text. Hashes where easy to spot because of the # and hexadecimal characters. Now names, in particular if they have spaces or are lowercased, can be hard to notice in the middle of the text. For example Spends from script foo (Reference Script at 28282838!0), or Pays to pubkey Clint Eastwood could become Pays to pubkey [Clint Eastwood] or Spends from script [foo], Pays to pubkey #"Clint Eastwood", etc. (I'd keep it like it is for now)

It could be good indeed to have names highlighted. Now that I think about it, there could be a different highlight if it's a script or a pubkey, which would also kinda solve the first question. Something like script@[myscript] and pkh@[mypkh]

Investigate using existential types or something to avoid having to split the names into several sublists for each concrete type. (That would be nice to have)

I'd say that would be nice to have but not mandatory. It practice, the only way I see that happening is by having a list of types as a type or something like that which would require type families.

There was a hardcoded check to print "Lovelace"/"Quick"/"Permanent" for the relevant currency symbols. Should this now be moved to the default name table (which would contain those 3 elements instead of being empty by default)? (I find it would be conceptually more elegant but maybe prone to accidental overwrites in practice)

I like the idea of having the 3 by default. And if in turn the user overrides it, well it's ok.

@florentc
Copy link
Member Author

This PR is ready for review. The first post has been edited accordingly.

Decisions regarding the discussion items:

  • Displaying "pubkey" and "script" as prefix: kept as before
  • Token name parsing featured: postponed, removed from this PR
  • Printing names: following various experiments, kept the humble non-decorated printing of names which turns out to be the most sensible
  • Heterogeneous association list: way too overkill for close to 0 benefit, the current API is already very convenient, better is the enemy of good, and not a smart strategic choice in terms of available time to spend on cooked
  • Moving hardcoded "Lovelace"/"Quick"/"Permanent" to default hash names map: done. Now there is a defaultHashNames the user can extend

In addition:

  • "Verbose mode" (see pcOptHashVerbose) matches the legacy style pubkey #a1b2c3d (some name) we used to have when wallet numbers were displayed
  • Speaking of wallet numbers, they are no longer displayed as the new feature supersedes it. Note that wallet numbers could be reintroduced in defaultHashNames
  • Changelog has been updated to include the new feature
  • The cheatsheet has a new entry that gives the same code snippet as the first post of this PR

@florentc florentc marked this pull request as ready for review March 19, 2024 15:34
@florentc florentc requested a review from mmontin March 19, 2024 15:39
@florentc florentc merged commit 8b8292e into main Mar 20, 2024
7 checks passed
@florentc florentc deleted the fix-379-pretty-address-names branch March 20, 2024 09:47
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.

2 participants