-
Notifications
You must be signed in to change notification settings - Fork 220
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
[ADP-2956] Upgrade cardano-node
to 8.1.1 and ghc
to 9.2.8
#4003
Conversation
42a327e
to
74b5e5d
Compare
4b8d9c5
to
6c24ec1
Compare
cardano-node
to 8.1.1 and ghc
to 9.2.8
cardano-node
to 8.1.1 and ghc
to 9.2.8
lib/primitive/test/spec/Cardano/Wallet/Primitive/CollateralSpec.hs
Outdated
Show resolved
Hide resolved
, _extraEntropy = Ledger.NeutralNonce | ||
|
||
let pparams = | ||
Ledger.emptyPParams |
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 now won't be notified if a new field is added which we need to provide a more sensible value for what emptyPParams
does. Looking at emptyPParams
it does seem some values are more sensible than just "0" though, so hopefully this isn't a problem we're going to run into in the future.
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 work! Lgtm modulo CI passing
, C.protocolParamCostModels = | ||
mempty | ||
-- TODO: Include a Plutus cost model here. | ||
, C.protocolParamPrices = | ||
Just $ C.ExecutionUnitPrices | ||
{ C.priceExecutionSteps = 7.21e-5 | ||
, C.priceExecutionMemory = 0.0577 | ||
, C.priceExecutionMemory = 0.057_7 |
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.
TBH, not sure I find this more readable
setGitRevPostInstall' = gitrev: '' ''; | ||
# The following is commented out because it causes error with | ||
# 'packages.cardano-node.package.identifier.name' not being defined. | ||
# setGitRevPostInstall' = gitrev: '' | ||
# ${pkgs.buildPackages.haskellBuildUtils}/bin/set-git-rev "${gitrev}" $out/bin/* |
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.
🆗
in { | ||
name = "cardano-wallet"; | ||
compiler-nix-name = "ghc8107"; | ||
compiler-nix-name = "ghc928"; |
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.
🎉
6a73756
to
ced54e6
Compare
import qualified Cardano.Wallet.Primitive.Types.Coin as W | ||
import qualified Cardano.Wallet.Primitive.Types.Coin as Coin | ||
import qualified Cardano.Wallet.Shelley.Compatibility.Ledger as Ledger |
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 plan was to inline *.Compatibility
into these more specialized modules — but this is still good enough for merging.
@@ -8,6 +8,7 @@ | |||
{-# LANGUAGE TypeApplications #-} | |||
|
|||
{-# OPTIONS_GHC -fno-warn-orphans #-} | |||
{-# OPTIONS_GHC -Wno-incomplete-uni-patterns #-} |
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 that this warning is somewhat out of scope for this pull request. 😅 But it's done now.
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.
it's for the specs to use partial functions ?
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.
Yes. (I'm not fond of that warning.) What I meant was that the specs worked before without this GHC_OPTIONS
pragma, but now they seem to need it, and this particular change seems out of scope to me as it touches a lot of modules.
But they have been touched now, I'm fine with proceeding.
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 to me — I can find no obvious defects! 😊
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 like a fix to the serialization issue to me. 😊
cec0d1f
to
b3c9299
Compare
Comments
This PR builds on top of 2 other PRs:
The #3959 is superseded by this one.
This is the squashed version of this PR: #3996
This PR introduced a huge regression in the restoration benchmark, which was solved by removing heap profiling. Doing so we have lost the nice memory usage graphs.
Issue Number
ADP-2956