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

Update cardano-api to 9.4.0.0 #936

Merged
merged 5 commits into from
Oct 16, 2024
Merged

Update cardano-api to 9.4.0.0 #936

merged 5 commits into from
Oct 16, 2024

Conversation

smelc
Copy link
Contributor

@smelc smelc commented Oct 14, 2024

Changelog

- description: |
    Update cardano-api to 9.4.0.0
# 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...

How to trust this PR

Review the parts highlighted in comments below 👇 (This is an important ... to review).

@carbolymer
Copy link
Contributor

Fixes: #754

@carbolymer carbolymer linked an issue Oct 14, 2024 that may be closed by this pull request
@smelc smelc force-pushed the smelc/update-api-to-9.4.0.0 branch from 635008c to 2a7e32e Compare October 14, 2024 15:01
@smelc smelc force-pushed the smelc/update-api-to-9.4.0.0 branch 2 times, most recently from 2003836 to b0cd6a8 Compare October 15, 2024 08:37
@@ -293,6 +297,16 @@ friendlyTxBodyImpl
friendlyLedgerProposals cOnwards proposalProcedures =
Array $ fromList $ map (friendlyLedgerProposal cOnwards) proposalProcedures

-- | API doesn't yet show that supplemental datums are alonzo onwards. So we do it in this prototype,
-- even if we don't use the witness.
friendlySupplementalDatums
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 an important function to review.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine. What you should do next is update the friendly rendering to use the experimental api. It will simplify things.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should render the datum hashes as well. See my other comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed from:

instance ToJSON HashableScriptData where
  toJSON = scriptDataToJsonDetailedSchema

to:

instance ToJSON HashableScriptData where
  toJSON hsd =
    object
      [ "bytes" .= (String $ Text.decodeUtf8 $ getOriginalScriptDataBytes hsd)
      , "json" .= scriptDataToJsonDetailedSchema hsd
      ]

Copy link
Contributor

Choose a reason for hiding this comment

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

No this is not that useful. We want the hash otherwise users will need to calculate the hash themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry I didn't understood that the first time. Did this change:

@@ -29,6 +28,6 @@ instance (L.EraTxOut ledgerera, L.EraGov ledgerera) => ToJSON (L.NewEpochState l
 instance ToJSON HashableScriptData where
   toJSON hsd =
     object
-      [ "bytes" .= String (Text.decodeUtf8 $ getOriginalScriptDataBytes hsd)
+      [ "hash" .= hashScriptDataBytes hsd
       , "json" .= scriptDataToJsonDetailedSchema hsd
       ]

@@ -23,3 +24,6 @@ instance (L.EraTxOut ledgerera, L.EraGov ledgerera) => ToJSON (L.NewEpochState l
, "rewardUpdate" .= nesRu
, "currentStakeDistribution" .= nesPd
]

instance ToJSON HashableScriptData where
toJSON = scriptDataToJsonDetailedSchema
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 an important choice to review.

Copy link
Contributor

@carbolymer carbolymer Oct 15, 2024

Choose a reason for hiding this comment

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

Vide comment from cardano-api:

-- | Warning: Creating 'HashableScriptData' from a 'ScriptData' value pretty
-- much guarantees the original bytes used to create the 'ScriptData'
-- value will be different if we serialize `HashableScriptData` again.
-- Do not use this.
unsafeHashableScriptData :: ScriptData -> HashableScriptData
unsafeHashableScriptData sd = HashableScriptData (serialiseToCBOR sd) sd

I understand that it means that we can't be sure that deserialisation from JSON representation into HashableScriptData will generate the value with the same CBOR representation. (why?)

If that is the case here, we should prohibit from deserialising from JSON I think, for example like that:

instance TypeError (Text "Cannot deserialise HashableScriptData." :$$:
                    Text "Some good justification")
      => FromJSON HashableScriptData where
fromJSON = error "unreachable"

Copy link
Contributor

@Jimbo4350 Jimbo4350 Oct 15, 2024

Choose a reason for hiding this comment

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

I understand that it means that we can't be sure that deserialisation from JSON representation into HashableScriptData will generate the value with the same CBOR representation.

Ledger's CBOR serialization is not guaranteed to round trip and this can change the hash. See IntersectMBO/cardano-ledger#2943 (comment)

However I don't see the issue here. scriptDataToJsonDetailedSchema does not call unsafeHashableScriptData. If we were rendering the hash (which we should) then we have to be careful and use hashScriptDataBytes.

Copy link
Contributor

@carbolymer carbolymer Oct 16, 2024

Choose a reason for hiding this comment

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

@Jimbo4350 there's no issue yet. It's about making hidden assumptions clear. If someone writes naive FromJSON instance those two expressions could give different results:

hsd = undefined :: HashableScriptData

Right $ hashScriptDataBytes hsd

and

(hashScriptDataBytes . scriptDataFromJsonNoSchema) <$> (Aeson.eitherDecode $ Aeson.encode hsd)

My point is to make that non-roundtripping assumption clearer, or somehow expressed in the type system.

@smelc smelc marked this pull request as ready for review October 15, 2024 08:41
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

LGTM, but i'm leaving an approval to @Jimbo4350 since I'm not sure about the two places you pointed out.

cardano-cli/src/Cardano/CLI/Json/Friendly.hs Outdated Show resolved Hide resolved
@@ -23,3 +24,6 @@ instance (L.EraTxOut ledgerera, L.EraGov ledgerera) => ToJSON (L.NewEpochState l
, "rewardUpdate" .= nesRu
, "currentStakeDistribution" .= nesPd
]

instance ToJSON HashableScriptData where
toJSON = scriptDataToJsonDetailedSchema
Copy link
Contributor

@carbolymer carbolymer Oct 15, 2024

Choose a reason for hiding this comment

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

Vide comment from cardano-api:

-- | Warning: Creating 'HashableScriptData' from a 'ScriptData' value pretty
-- much guarantees the original bytes used to create the 'ScriptData'
-- value will be different if we serialize `HashableScriptData` again.
-- Do not use this.
unsafeHashableScriptData :: ScriptData -> HashableScriptData
unsafeHashableScriptData sd = HashableScriptData (serialiseToCBOR sd) sd

I understand that it means that we can't be sure that deserialisation from JSON representation into HashableScriptData will generate the value with the same CBOR representation. (why?)

If that is the case here, we should prohibit from deserialising from JSON I think, for example like that:

instance TypeError (Text "Cannot deserialise HashableScriptData." :$$:
                    Text "Some good justification")
      => FromJSON HashableScriptData where
fromJSON = error "unreachable"

@smelc smelc force-pushed the smelc/update-api-to-9.4.0.0 branch from c64a854 to 262f4d0 Compare October 15, 2024 14:33
@smelc smelc force-pushed the smelc/update-api-to-9.4.0.0 branch from 262f4d0 to 4e01efc Compare October 16, 2024 08:57
@smelc smelc force-pushed the smelc/update-api-to-9.4.0.0 branch from 4e01efc to 12f4c62 Compare October 16, 2024 09:05
@smelc smelc force-pushed the smelc/update-api-to-9.4.0.0 branch from 12f4c62 to a27d5f8 Compare October 16, 2024 12:22
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.

LGTM!

@smelc smelc enabled auto-merge October 16, 2024 12:40
@smelc smelc added this pull request to the merge queue Oct 16, 2024
Merged via the queue into main with commit 9d6f634 Oct 16, 2024
25 checks passed
@smelc smelc deleted the smelc/update-api-to-9.4.0.0 branch October 16, 2024 13:31
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.

TX balancing ignores assets in collateral inputs
4 participants