Support decoding list/indefinite list from cbor #331
Conversation
…to bugfix/extended-cip8
Bugfix/bytestring
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #331 +/- ##
==========================================
- Coverage 83.98% 83.92% -0.06%
==========================================
Files 29 29
Lines 3733 3757 +24
Branches 941 946 +5
==========================================
+ Hits 3135 3153 +18
- Misses 433 436 +3
- Partials 165 168 +3 ☔ View full report in Codecov by Sentry. |
|
I should mention I found a few other things as well. Most importantly, this should not be merged in its current state. It might be appropriate to convert this to WIP until the There seemed to be type errors in code I did not touch. It might be related to the changes I made, but it could also be due to me updating packages. |
There seem to be some type errors in the static analyzer. Either there is a bug or the analyzer needs more/better type hints |
nielstron
changed the title
nielstron
changed the title
|
Yes, that is exactly what I was talking about. I'm not entirely sure how those type errors got injected into this with my changes. On the bright side, I do know this works because I've been using it. I'm trying to get the attention of the cbor2 maintainer to merge the fix. |
Not sure what is happening in datum-options - the types seem wrong?
|
I guess suddenly the return type of cbor2.encode which was previously untyped came into scope and unveiled a bunch of mismatching types :) |
…to bugfix/datum
…nto bugfix/datum
…nto bugfix/datum
Refactor Redeemer handling in TransactionWitnessSet
…nto bugfix/datum
…into bugfix/datum
…nto bugfix/datum
…into bugfix/datum
…nto bugfix/datum
…to bugfix/datum
|
Revisiting this PR.. @theeldermillenial is it still WIP, or we can prepare for a merge? |
|
@cffls Unfortunately still WIP. The maintainer of cbor2 wants to make sure that the fix I made has feature parity with the C extension. Unfortunately, that is something that will take more time than I currently have. I am going to be adding someone to my team, and hopefully they can try to tackle this or free up enough of my time to fix it. |
tl;dr:
This PR adds support for proper parsing of fixed vs indefinite lists from CBOR.
Background
Currently, pycardano uses a custom encoder to embed indefinite lists when encoding classes to cbor, even if the length of the list is less than 31 elements.
This creates an issue when decoding with
cbor2, becausecbor2decodes all lists to thelistclass without any indication of whether the list was encoded as a fixed/indefinite list. This loss of information is problematic because hashes are frequently generated off of CBOR, so encoding as a fixed list vs indefinite list generates a different hash even if the content of the datum is the same.Implementation Detail
This PR is unfortunately dependent on a minor fix in
cbor2. TheCBORDecoderclass doesn't have a custom decoding mechanism to make modifications to how data is decoded. One approach to fixing this issue would be to subclassCBORDecoderto intervene when an indefinite list is found. However, theCBORDecoderwas implemented in such a way that it relies on a global dictionary that references internal method definitions on a class, making subclassing challenging. To resolve this issue a PR was opened tocbor2that simply moves the global dictionaries inside theCBORDecoderclass, which has no impact on any other section of code:agronholm/cbor2#225
This PR takes advantage of that PR to subclass the
CBORDecoderand intervene in the case of an indefinite list. When an indefinite list is discovered, it is cast toIndefiniteListand returned, while all other lists are returned as a standardlist. This required some additional changes, specifically a change how data is being decoded on thepycardanoside.This should fix a few different Issues, and fixed a previously undiscovered issue related to roundtrip cbor encoding of datums (datum -> cbor ->datum -> cbor).
fixes #311
fixes #330