Rewrite tokenBundleSizeAssessor with ledger types #4098
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Anviking
commented
Aug 16, 2023
lib/wallet/test/unit/Cardano/Wallet/Write/Tx/Balance/TokenBundleSizeSpec.hs
Show resolved
Hide resolved
lib/wallet/src/Cardano/Wallet/Write/Tx/Balance/TokenBundleSize.hs
Outdated
Show resolved
Hide resolved
Anviking
force-pushed
the
anviking/ADP-3081/ledger-tokenBundleSizeAssessor
branch
from
740ee3d to
d472455
Compare
This removes the dependency on `Cardano.Wallet.Shelley.Compatibility.toCardanoValue`
Anviking
force-pushed
the
anviking/ADP-3081/ledger-tokenBundleSizeAssessor
branch
from
d472455 to
bf53694
Compare
Anviking
commented
Sep 11, 2023
lib/wallet/test/unit/Cardano/Wallet/Write/Tx/Balance/TokenBundleSizeSpec.hs
Outdated
Show resolved
Hide resolved
Anviking
commented
Sep 11, 2023
lib/wallet/test/unit/Cardano/Wallet/Write/Tx/Balance/TokenBundleSizeSpec.hs
Outdated
Show resolved
Hide resolved
|
@jonathanknowles I noticed some remaining things, but could you please have a look? A key change here is that we now use the entire |
There was a problem hiding this comment.
Hi @Anviking. This looks mostly good to me -- I've made some suggestions.
lib/wallet/test/unit/Cardano/Wallet/Write/Tx/Balance/TokenBundleSizeSpec.hs
Outdated
Show resolved
Hide resolved
lib/wallet/src/Cardano/Wallet/Write/Tx/Balance/TokenBundleSize.hs
Outdated
Show resolved
Hide resolved
lib/wallet/test/unit/Cardano/Wallet/Write/Tx/Balance/TokenBundleSizeSpec.hs
Outdated
Show resolved
Hide resolved
lib/wallet/test/unit/Cardano/Wallet/Write/Tx/Balance/TokenBundleSizeSpec.hs
Outdated
Show resolved
Hide resolved
lib/wallet/src/Cardano/Wallet/Write/Tx/Balance/TokenBundleSize.hs
Outdated
Show resolved
Hide resolved
lib/wallet/src/Cardano/Wallet/Write/Tx/Balance/TokenBundleSize.hs
Outdated
Show resolved
Hide resolved
lib/wallet/src/Cardano/Wallet/Write/Tx/Balance/TokenBundleSize.hs
Outdated
Show resolved
Hide resolved
2 tasks
Anviking
commented
Sep 13, 2023
| @@ -64,17 +88,17 @@ spec = describe "Assessing the sizes of token bundles" $ do | |||
| prop_assessTokenBundleSize_enlarge | |||
| :: Blind (VariableSize1024 TokenBundle) | |||
| -> Blind (VariableSize16 TokenBundle) | |||
| -> PParamsInRecentEra | |||
There was a problem hiding this comment.
Hm, it seems this was hard-coded before, but it seems to work 🤷♂️
Anviking
commented
Sep 13, 2023
| bundle | ||
| actualLengthBytes = computeTokenBundleSerializedLengthBytes bundle | ||
| actualAssessment = assessTokenBundleSize assessor bundle | ||
| v = eraProtVerLow @StandardBabbage -- FIXME! |
There was a problem hiding this comment.
I think just dropping the FIXME is fine. Ideally we'd re-use the same version as was used by the TokenBundleSizeAssessor but 🤷♂️
lib/wallet/test/unit/Cardano/Wallet/Write/Tx/Balance/TokenBundleSizeSpec.hs
Outdated
Show resolved
Hide resolved
lib/wallet/test/unit/Cardano/Wallet/Write/Tx/Balance/TokenBundleSizeSpec.hs
Outdated
Show resolved
Hide resolved
| & ppProtocolVersionL .~ (ProtVer (eraProtVerLow @StandardBabbage) 0) | ||
| & ppMaxValSizeL .~ maryTokenBundleMaxSize | ||
| where | ||
| maryTokenBundleMaxSize = 4000 |
There was a problem hiding this comment.
I think we should be clear about the units here:
Suggested change
| maryTokenBundleMaxSize = 4000 | |
| maryTokenBundleMaxSizeBytes = 4000 |
By the way, this seems to have recently changed to 5000:
There was a problem hiding this comment.
Co-authored-by: Jonathan Knowles <[email protected]>
Anviking
force-pushed
the
anviking/ADP-3081/ledger-tokenBundleSizeAssessor
branch
from
e0cfdd8 to
270b6ad
Compare
jonathanknowles
approved these changes
Sep 14, 2023
Anviking
deleted the
anviking/ADP-3081/ledger-tokenBundleSizeAssessor
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes the dependency on
Cardano.Wallet.Shelley.Compatibility.toCardanoValueIssue Number
ADP-3081