-
Notifications
You must be signed in to change notification settings - Fork 7
ADR-016 Backwards and Forwards Compatibility #79
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍. I just wrote some suggestions to improve readability
|
||
I will make this clearer with an example below. | ||
|
||
**NB**: We will only implement functionality constrained by `IsShelleyBasedEra era` and `IsEra era` that either is currently exposed by the "old" api or specifically required by QA. If we do not need backwards compatibility then we will use`Era era` or `IsEra era` without implementing an additional type synonym family. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
**NB**: We will only implement functionality constrained by `IsShelleyBasedEra era` and `IsEra era` that either is currently exposed by the "old" api or specifically required by QA. If we do not need backwards compatibility then we will use`Era era` or `IsEra era` without implementing an additional type synonym family. | |
**NB**: We will only implement functionality constrained by `IsShelleyBasedEra era` and `IsEra era` that either is currently exposed by the "old" api or specifically required by QA. If we do not need backwards compatibility then we will use `Era era` or `IsEra era` without implementing an additional type synonym family. |
There is no space between "use" and "Era era
"
Ledger.mkDelegTxCert (Api.toShelleyStakeCredential sCred) delegatee | ||
|
||
|
||
--- BELOW IS IN A SEPARATE "COMPATIBLE" MODULE DESIGNED FOR BACKWARDS COMPATIBILITY. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This already implies that the function above is not backwards compatible, but I would say it explicitly, so that the difference is more obvious to the reader. I would even explicitly say that IsEra
constrains to the latest two eras while IsShelleyBasedEra
doesn't and thus provides backwards compatibility.
Also both versions could be in two separate Haskell code blocks.
-- The `StakeDelegationRequirements era` GADT will necessitate more data constructors if there are any changes to the requirements of stake delegation in future eras. | ||
-- This is already seen in the transition from Babbage (can only delegate to stake pools) -> Conway (can delegate to stake pools and dreps). | ||
-- This obviously does not scale in the face of possible incoming changes and requires the propagation of an additional data constructor. | ||
|
||
-- I propose the following: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These comments could be unquoted. I think that would make it easier to parse
-> Certificate era | ||
``` | ||
|
||
`Cardano.Api.Certificate` exposes several helper functions that use this "old" type. E.g `makeStakeAddressDelegationCertificate`, `makeStakeAddressRegistrationCertificate` and `makeStakeAddressUnregistrationCertificate` to name a few. All of these "old" functions will be deprecated in favour of a new API that allows us to expose functionality from any era required without adding significant boilerplate as seen in the above definition of `data Certificate era`. We achieve this by the following: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not link to IntersectMBO/cardano-api#962 as an example. The inline example is much better, but still, being able to easily check the other one could help too.
|
||
## Proposed Solution | ||
|
||
The solution proposed below is specific to certificates but can be expanded to any era depdendent type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution proposed below is specific to certificates but can be expanded to any era depdendent type. | |
The solution proposed below is specific to certificates but can be expanded to any era dependent type. |
|
||
I will make this clearer with an example below. | ||
|
||
**NB**: We will only implement functionality constrained by `IsShelleyBasedEra era` and `IsEra era` that either is currently exposed by the "old" api or specifically required by QA. If we do not need backwards compatibility then we will use`Era era` or `IsEra era` without implementing an additional type synonym family. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: use [!NOTE]
admonition instead of NB https://github.com/orgs/community/discussions/16925 for better visual separation.
:: forall era | ||
. IsShelleyBasedEra era | ||
=> StakeCredential | ||
-> Delegatee era |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> Delegatee era | |
-> Delegatee (LedgerEra era) |
:: forall era | ||
. IsEra era | ||
=> StakeCredential | ||
-> Delegatee era |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> Delegatee era | |
-> Delegatee (LedgerEra era) |
`Cardano.Api.Certificate` exposes several helper functions that use this "old" type. E.g `makeStakeAddressDelegationCertificate`, `makeStakeAddressRegistrationCertificate` and `makeStakeAddressUnregistrationCertificate` to name a few. All of these "old" functions will be deprecated in favour of a new API that allows us to expose functionality from any era required without adding significant boilerplate as seen in the above definition of `data Certificate era`. We achieve this by the following: | ||
|
||
|
||
Replace era-variant GADTs (like Certificate era) with a uniform wrapper (Certificate :: L.TxCert era -> Certificate era) and using open type families to resolve era-specific types (e.g., Delegatee era). Era constraints (IsEra, IsShelleyBasedEra) gate access to functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using open type families to resolve era-specific types (e.g., Delegatee era)
The example below uses a closed type family Delegatee era
. What's the final choice, and why?
Delegatee ConwayEra = Ledger.Delegatee | ||
Delegatee BabbageEra = Api.Hash Api.StakePoolKey | ||
Delegatee AlonzoEra = Api.Hash Api.StakePoolKey | ||
... | ||
... | ||
Delegate ShelleyEra = Api.Hash Api.StakePoolKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delegatee ConwayEra = Ledger.Delegatee | |
Delegatee BabbageEra = Api.Hash Api.StakePoolKey | |
Delegatee AlonzoEra = Api.Hash Api.StakePoolKey | |
... | |
... | |
Delegate ShelleyEra = Api.Hash Api.StakePoolKey | |
Delegatee L.ConwayEra = Ledger.Delegatee | |
Delegatee L.BabbageEra = Api.Hash Api.StakePoolKey | |
Delegatee L.AlonzoEra = Api.Hash Api.StakePoolKey | |
... | |
... | |
Delegate L.ShelleyEra = Api.Hash Api.StakePoolKey |
Delegatee
is a part of the experimental (new) API so I think we should be using ledger eras.
No description provided.