- 
                Notifications
    You must be signed in to change notification settings 
- Fork 25
Upgrade ledger, consensus, network, plutus for cardano-node 10.6 #954
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
Conversation
9796d54    to
    f83f795      
    Compare
  
    ffca5b1    to
    4741b1d      
    Compare
  
    cf3825b    to
    833aa4e      
    Compare
  
    - QueryAccountState returns `ChainAccountState` instead of `AccountState` - Use updated consensus queries `GetStakeDistribution2` and `GetPoolDistr2`
`ConstitutionPurpose`
6cfae60    to
    aa2acde      
    Compare
  
    82d12a6    to
    7b990ba      
    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.
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.
LGTM, few minor comments
| Map.toList $ | ||
| fromJust $ | ||
| extractCostModelParamsLedgerOrder mCostModel | ||
| -- all geneses should contain only the number of cost model params equal to the initial number | 
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.
What does this mean?
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.
rephrased
| LedgerProtocolParameters pp <- Ledger.maybeToStrictMaybe mTxProtocolParams | ||
| -- This logic is copied from ledger, because their code is not reusable | ||
| -- c.f. https://github.com/IntersectMBO/cardano-ledger/commit/5a975d9af507c9ee835a86d3bb77f3e2670ad228#diff-8236dfec9688f22550b91fc9a87af9915523ab9c5bd817218ecceec8ca7a789bR282 | ||
| let shouldCalculateHash = | 
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.
Seems like an unnecessary check. What's the benefit of calling guard here vs letting hashScriptIntegrity return SNothing? The guard is guarding the guard.
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.
Because hashScriptIntegrity is changed now and does not return SNothing.
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 can use mkScriptIntegrity. Not important for this PR though.
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 could, but you'd have to build whole Tx era to use it, which results in a lot of boilerplate. We are using convPParamsToScriptIntegrityHash before we start building a transaction so it'd result in a code duplication. Tbh it would be easier to open a PR to ledger to undo requirement of Tx era.
|  | ||
| prop_text_envelope_roundtrip_tx_CBOR :: Property | ||
| prop_text_envelope_roundtrip_tx_CBOR = H.property $ do | ||
| _prop_text_envelope_roundtrip_tx_CBOR :: Property | 
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.
Do we still want this disabled?
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.
reenabled
| stakeCred <- forAll genStakeCredential | ||
| -- that's the same stake credential as in UTXO | ||
| lStakeCred@(L.KeyHashObj kh) <- | ||
| pure $ mkCredential "keyHash-ebe9de78a37f84cc819c0669791aa0474d4f0a764e54b9f90cfe2137" | 
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.
Given that (as I understand it) the fee increase is deterministic can we go back to using genStakeCredential?
Changelog
Context
Dependencies upgrade in preparation for node 10.6 release
Checklist