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

CIP-0129 support #288

Open

paweljakubas wants to merge 19 commits into master from paweljakubas/284/cip-0129-support

Collaborator

paweljakubas commented Dec 16, 2024 •

edited

Loading

PR adds CIP-0129 support

Basically drep, cc hot and cc cold key hashes are decorated with prepended byte. The same is for script hashes.
It affects data payload that is used as hashes. For the rest key hashes and script hashes it is 28-byte payload, here 29-byte payload.

Tackles #284

There is going to be additional PR enhancing https://github.com/cardano-foundation/CIPs/tree/master/CIP-0105 test vectors and deprecation comments

In the end we are going to have the following behavior

At this moment we support credentials
X : payload is specific byte + 28-byte key hash
X : payload is specific byte+ 28-byte script hash

We are going to support additionally
X: payload is 28-byte key hash (deprecated)
X_script: payload is 28-byte script hash
X_vkh : payload is 28-byte key hash

where
X={drep, cc_cold, cc_hot}

paweljakubas self-assigned this

paweljakubas force-pushed the paweljakubas/284/cip-0129-support branch from a316308 to 7a3a998 Compare

December 16, 2024 17:00

Contributor

abailly commented Dec 17, 2024

@paweljakubas If you replace Tackles https://github.com/IntersectMBO/cardano-addresses/issues/284 with Fix https://github.com/IntersectMBO/cardano-addresses/issues/284 in the description of your PR, the issue will be automatically closed when the PR is merged.

abailly reviewed

View reviewed changes

Contributor

abailly left a comment

That provides a very good coverage of the features, I would like to suggest we remove some duplication in order to improve the maintainability of this code over time

command-line/lib/Command/Key/Hash.hs

@@ @@ -85,6 +87,22 @@ run Hash{outputFormat} = do @@
                   allowedPrefixes = map fst prefixes
                   prefixFor = fromJust . flip lookup prefixes
+                  afterHashing hrp hashed

Contributor

abailly Dec 19, 2024

Wouldn't pattern matching be a more elegant solution than a sequence of ifs here?

command-line/lib/System/IO/Extra.hs

+              -- | Print string to the console without newline appended.
+              hPutStringNoNewLn :: Handle -> String -> IO ()
+              hPutStringNoNewLn h =
+                  B8.hPutStr h . T.encodeUtf8 . T.pack

Contributor

abailly Dec 19, 2024

This feels weird, why not use directly System.IO.hPutStr?

Contributor

abailly Dec 19, 2024

Because of strictness perhaps?

command-line/test/Command/Script/HashSpec.hs

@@ @@ -44,85 +44,85 @@ spec = do @@
                       specScriptHashProper "script1w8469gq5ed7xtyf2tqdng5yn7ykgckkfcl38xre8hk3ejk2lcwt"
                           [iii|#{verKeyH4}|]
-                      specScriptHashProper "drep_script13hu7a632tuntrlyjkazzczrzzt7dx6e8v3rc25jd9zcgv68nx9r"
+                      specScriptHashProper "drep1ydv8xcdxwpy7zrf805yenpz7cdnap4c87v7zewvmdjyyygq44jy4j"

Contributor

abailly Dec 19, 2024

Looking at those strings makes me wish those were in a text file usable externally and loaded by our tests so as to be relatively simple to edit and publish

core/lib/Cardano/Address/Script.hs Outdated

Comment on lines 255 to 265

+                  appendByte payload = case cred of
+                      Representative ->
+                          let fstByte = 0b00100011 :: Word8
+                          in BS.cons fstByte payload
+                      CommitteeCold ->
+                          let fstByte = 0b00010011 :: Word8
+                          in BS.cons fstByte payload
+                      CommitteeHot ->
+                          let fstByte = 0b00000011 :: Word8
+                          in BS.cons fstByte payload
+                      _ -> payload

Contributor

abailly Dec 19, 2024

Suggested change

      
                appendByte payload = case cred of
          
                    Representative ->
          
                        let fstByte = 0b00100011 :: Word8
          
                        in BS.cons fstByte payload
          
                    CommitteeCold ->
          
                        let fstByte = 0b00010011 :: Word8
          
                        in BS.cons fstByte payload
          
                    CommitteeHot ->
          
                        let fstByte = 0b00000011 :: Word8
          
                        in BS.cons fstByte payload
          
                    _ -> payload
          
                appendByte payload =  maybe payload (`BS.cons` payload) bytePrefix
          
                bytePrefix = case cred of
          
                    Representative -> Just 0b00100011 
          
                    CommitteeCold -> Just 0b00010011
          
                    CommitteeHot -> Just 0b00000011
          
                    _ -> Nothing

Contributor

abailly Dec 19, 2024

Also I would move toBytePrefix function cloes to the KeyRole type, possibly with a fromBytePrefix counterpart so that we can reuse them

core/lib/Cardano/Address/Script.hs

+                            let (fstByte, payload) = first BS.head $ BS.splitAt 1 bytes
+                            --   drep          0010....
+                            --   scripthash    ....0011
+                            in if fstByte == 0b00100011 then

Contributor

abailly Dec 19, 2024

that's where we could reuse toBytePrefix/fromBytePrefix perhaps?

core/lib/Cardano/Address/Script.hs

+                              >>= maybeToRight ErrScriptHashFromTextWrongPayload . scriptHashFromBytes
+               where
+                  convertBytes hrp bytes
+                      | hrp == CIP5.drep && checkBSLength bytes 29 =

Contributor

abailly Dec 19, 2024

Lots of duplication in there :)

core/lib/Cardano/Address/Script.hs Outdated

+              --
+              --   keyhash    ....0010
+              keyHashAppendByteCIP0129 :: ByteString -> KeyRole -> ByteString
+              keyHashAppendByteCIP0129 payload = \case

Contributor

abailly Dec 19, 2024

This duplicates the appendByte function above, factor out?

core/lib/Cardano/Address/Script.hs

+              -- Possible errors when deserializing a script hash from text.
+              --
+              -- @since 4.0.0
+              data ErrScriptHashFromText

Contributor

abailly Dec 19, 2024

I think it would be useful to provide more details to the user when something fails, like the invalid string, or the wrong prefix, etc.

paweljakubas added 19 commits

December 24, 2024 10:32


          clean bech32 prefixes

5c3b465


          adjustment to keyHashToText and keyHashFromText

79fccce


          introduce scriptHashToText and clean Command.Hash.Script

32ed34a


          fixing testing 1

7fdb598


          scriptHashFromText impl

e37e7cb


          simplify scriptHashReader and add prettyErrScriptHashFromText

27b9dbd


          fix testing - 2

e2a81cb


          adjust cmd key hash to CIP-0129

2386fff


          fix all cmd script hash tests

b07e451


          fix all preimage tests

249494b


          fix script validate tests

e65e4ea


          fix ShelleySpec unit tests

ee3480f


          update Chengelog

90e3660


          coding stanrdards

9a40047

remove redundant trace


          refactor scriptHashToText

eedcb75


          add Governance applicative and fix use of scriptHashToText in cmd

7b50d7e


          keyHashToTxt adjustment

cb89e5f


          adjust Command.Key.Hash

7e65db4


          fix cli testing

2b8ac69

paweljakubas force-pushed the paweljakubas/284/cip-0129-support branch from 7a3a998 to 2b8ac69 Compare

December 24, 2024 10:58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet