-
Notifications
You must be signed in to change notification settings - Fork 720
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
New NodeToClientVersionOf typeclass #4787
Conversation
071959e
to
5e85d20
Compare
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.
Nice, a few comments
cardano-api/src/Cardano/Api/Eras.hs
Outdated
@@ -215,6 +218,9 @@ data AnyCardanoEra where | |||
=> CardanoEra era -- and explicit value. | |||
-> AnyCardanoEra | |||
|
|||
instance NtcVersionOf AnyCardanoEra where | |||
ntcVersionOf (AnyCardanoEra _) = NodeToClientV_9 |
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 are we defaulting to NodeToClientV_9
for any era? Shouldn't we be explicit with each era and assign the version as follows:
--
data NodeToClientVersion
= NodeToClientV_9
-- ^ enabled @CardanoNodeToClientVersion7@, i.e., Alonzo
| NodeToClientV_10
-- ^ added 'GetChainBlockNo' and 'GetChainPoint' queries
| NodeToClientV_11
-- ^ added 'GetRewardInfoPools` Block query
| NodeToClientV_12
-- ^ added 'LocalTxMonitor' mini-protocol
| NodeToClientV_13
-- ^ enabled @CardanoNodeToClientVersion9@, i.e., Babbage
| NodeToClientV_14
-- ^ added @GetPoolDistr, @GetPoolState, @GetSnapshots
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.
I.e NodeToClientV_12
for everything up to Alonzo and NodeToClientV_14
for Babbage? The downside is we have to manually maintain this pairing.
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 are we defaulting to NodeToClientV_9 for any era?
NodeToClientV_9
is the oldest era that is still being tracked. Any queries that exist but are not in the above list above are presume to be from an era older than NodeToClientV_9
, but we set it to NodeToClientV_9
because it is long enough ago that we don't care about the precise version anymore.
Note that our API doesn't export everything. For example GetRewardInfoPools
is never used in any of our code.
The downside is we have to manually maintain this pairing.
Yeah, I believe some form of this might belong in consensus. It's implemented here for the moment because getting changes in consensus can take a long time.
Even if it is implemented in consensus, some form of this will exist in our code because the type of the argument to ntcVersionOf
is a type that exists in our code base.
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.
NodeToClientV_9 is the oldest era that is still being tracked
Sure but the NodeToClientV_*
depends on the era, so I don't think we should always default to NodeToClientV_9
. E.g as things are implemented now if we call ntcVersionOf
on AnyCardanoEra BabbageEra
there are more queries available in Babbage than NodeToClientV_9
will allow.
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.
That's an interesting point. Would it be sufficient for nodeToClientVersionOf
to take an era argument?
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.
Note that we won't know the node's era unless we query for it, so it's not free.
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.
I think manually doing this for now is ok.
a2745dd
to
0eb8f0a
Compare
8421f08
to
59c6a02
Compare
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.
0b07afd
to
76246af
Compare
76246af
to
f0c2c02
Compare
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.
Still a couple of import changes needed
33e927b
to
e4b51de
Compare
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.
Sorry, instance NodeToClientVersionOf AnyCardanoEra where
needs to be updated to reflect the latest versions for the relevant era
0dba35c
to
ddee4d2
Compare
Removed unused typeclass instance
ddee4d2
to
2aa95b0
Compare
I think there is not a strong relationship between the node to client version and the era. For example a query could be added during an era, but depending on the circumstances, the added query might work for an earlier 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.
LGTM
@@ -58,7 +58,6 @@ import Data.Type.Equality (TestEquality (..), (:~:) (Refl)) | |||
import Ouroboros.Consensus.Shelley.Eras as Consensus (StandardAllegra, StandardAlonzo, | |||
StandardBabbage, StandardMary, StandardShelley) | |||
|
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.
Random diff
2aa95b0
to
7c55c61
Compare
7c55c61
to
b5b1f5d
Compare
This allows code to check what version of the node to client protocol is needed to use the query. For example:
Whilst this function can be called directly, it is intended to be called by
queryExpr
eventually so that only queries that are valid for the current protocol are ever sent.