Quick fix for #1561 #1562
Quick fix for #1561 #1562
Conversation
|
Thanks for the PR @Quantumplation, the fix LGTM. We use conventional commits on the repo since we pull release notes automatically from it, could you rename the first line of the commit to something like:
Or similar? thanks! |
Quantumplation
force-pushed
the
pi/1561/cbor-issues
branch
4 times, most recently
from
6445af9 to
d1c3dc0
Compare
This is a very quick fix for input-output-hk#1561 Because the conditions used in `getMapSize` don't match those used in `toCbor`, the cbor can end up invalid: for example, the map size could be written as 8, but then only 7 values are written, if totalCollateral is 0, for example. A more robust solution would be to extend CborWriter to maintain a stack of buffers, and add `startDefiniteLengthMap` and `endDefiniteLengthMap` methods. However, we're currently blocked by this, so I did the quickest thing possible.
Quantumplation
force-pushed
the
pi/1561/cbor-issues
branch
from
d1c3dc0 to
3257320
Compare
|
Ok there we go; sorry about the noise, couldn't get the signed commit working because of a mismatched email 😅 |
We need one more review, the team should be getting online in a few hours, once CI is green and we get the additional review, I will merge and cut a new release |
|
Thank you Angel! fwiw the reason this came up is because of some work we're doing for the Hydra Doom tournament next week :) |
Awesome stuff!, I figured as much. I have was looking the other day at the hydra provider yHSJ added to Blaze butaneprotocol/blaze-cardano#230. Really cool, I will try to port this provider to cardano-c when I have some spare time :) |
There was a problem hiding this comment.
Hi, thank you for contribution! ❤️ Would be great to add some unit test. A simplest one is a roundtrip test - would it reproduce #1561?
|
Given that this is time sensitive, and I'm unfamiliar with the codebase, can we merge without the test or could I ask you to write a test for it? I'm happy to follow up, get familiar with the testing framework, and add a test once we've got things shored up and working for our deadline 😅 |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Please cherry-pick a few more fixes and the unit tests from bb617b8 @Quantumplation .
I pushed them to this branch pr/Quantumplation/1562-sdk-tests
I will merge the updates separately. Good to go. Fantastic work @Quantumplation 🚀
Context
See #1561
Proposed Solution
This is a quick fix, which matches the conditionals in
toCborto the conditionals ingetMapSize; See #1561 and commit message for a more robust approach.Important Changes Introduced