-
Notifications
You must be signed in to change notification settings - Fork 721
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
Fix query protocol-state #4102
Fix query protocol-state #4102
Conversation
For Alonzo:
|
For Babbage:
|
b70bfc7
to
37b2a92
Compare
Verified the behavior in both Alonzo and Babbage, LGTM 👍 |
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.
Couple of small change but LGTM otherwise
import Cardano.Api.Orphans () | ||
|
||
import Cardano.Ledger.AuxiliaryData (AuxiliaryDataHash (..)) |
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.
Can you undo the import changes to follow the style in other modules?
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 re-organised the imports, but any chance we could use ImportQualifiedPost
language extension like we do in plutus-apps
so I could just do a line sort to keep my imports tidy?
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.
We would need to make a decision to use ImportQualifiedPost
in the repo. Having a mix of some modules using it and others not is not ideal. I personally don't see justification for this aesthetic change.
@@ -58,9 +59,9 @@ deriving newtype instance ToJSON ByronHash | |||
-- This instance is temporarily duplicated in cardano-config | |||
|
|||
instance ToJSON (HeaderHash blk) => ToJSON (Tip blk) where | |||
toJSON TipGenesis = object [ "genesis" .= True ] | |||
toJSON TipGenesis = J.object [ "genesis" .= True ] |
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 the qualified import? If you are going to qualify aeson imports we do it via Aeson
. In this case I think its overkill to qualify object
as it is fairly obvious already that it comes from the aeson library.
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've switched it to Aeson
. I'm trying to avoid non-explicit imports and at the same time keep the qualified import short so it doesn't wrap.
37b2a92
to
8557561
Compare
8557561
to
4590123
Compare
bors r+ |
Build succeeded: |
Fixes one of the commands in #3883