-
Notifications
You must be signed in to change notification settings - Fork 12
fix: refinements in decoding msgpack and minor bug fixes API generator #215
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
ddefbb3 to
123f0a4
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.
Pull Request Overview
This PR refactors msgpack decoding to correctly handle raw bytes and non-string map keys, fixes boolean fields in the block model, and synchronizes API field naming with the latest OpenAPI specification. The changes address data integrity issues when decoding msgpack responses with non-UTF-8 decodable keys and values.
- Changes msgpack unpacking from
raw=Falsetoraw=Trueto preserve byte data integrity - Updates block model to use
bytesandintkeys for state deltas instead ofstrkeys - Corrects API generator to use
x-algokit-field-renamefor Python field names while preserving wire names
Reviewed Changes
Copilot reviewed 30 out of 31 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/algokit_algod_client/client.py | Updates msgpack decoding to use raw=True and adds byte-to-string conversion logic |
| src/algokit_indexer_client/client.py | Updates msgpack decoding to use raw=True and adds byte-to-string conversion logic |
| src/algokit_kmd_client/client.py | Updates msgpack decoding to use raw=True and adds byte-to-string conversion logic |
| src/algokit_algod_client/models/block.py | Changes state delta and state proof tracking keys from str to bytes/int with proper encoders/decoders |
| src/algokit_algod_client/models/_teal_key_value.py | Changes key field from str to bytes with base64 encoding |
| src/algokit_algod_client/models/_eval_delta_key_value.py | Changes key field from str to bytes with base64 encoding |
| src/algokit_algod_client/models/_pending_transaction_response.py | Corrects wire names for app_id and asset_id fields |
| src/algokit_algod_client/models/_get_block_tx_ids_response_model.py | Renames class and field to match operation ID specification |
| src/algokit_indexer_client/models/_transaction.py | Renames fields to created_app_id and created_asset_id |
| src/algokit_indexer_client/models/_asset.py | Renames index field to id_ |
| src/algokit_algod_client/models/_asset.py | Renames index field to id_ |
| src/algokit_algod_client/models/_dryrun_source.py | Renames app_index field to app_id |
| src/algokit_algod_client/models/_app_call_logs.py | Corrects wire name for app_id field |
| src/algokit_algod_client/models/_genesis_file_in_json.py | Makes timestamp field optional |
| src/algokit_transact/models/state_proof.py | Changes reveals from tuple to dict[int, Reveal] with improved encoding/decoding |
| src/algokit_algod_client/models/_source_map.py | Adds new SourceMap model |
| src/algokit_algod_client/models/_teal_compile_response_model.py | Changes sourcemap from generic dict to SourceMap model |
| src/algokit_*_client/models/_serde_helpers.py | Adds key_encoder/key_decoder parameters to mapping functions, updates TypeVar names |
| api/oas-generator/src/oas_generator/builder.py | Fixes field rename logic to use x-algokit-field-rename for Python names |
| tests/modules/algod_client/test_block.py | Updates test to cover blocks with non-UTF-8 decodable keys |
| tests/modules/transact/common.py | Updates _parse_reveals to return dict instead of tuple |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
api/oas-generator/src/oas_generator/renderer/templates/models/_serde_helpers.py.j2
Outdated
Show resolved
Hide resolved
123f0a4 to
ce452e0
Compare
06694b3 to
5be11b0
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.
Pull Request Overview
Copilot reviewed 36 out of 37 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/algokit_algod_client/models/_block.py:80
- The boolean check at line 73-74 is problematic because in Python,
boolis a subclass ofint, soisinstance(key, bool)must be checked beforeisinstance(key, int). However, the current order is correct. The issue is that this function allows boolean input which seems incorrect for 'local delta index keys' that should be numeric. If booleans are intentionally supported, this should be documented; otherwise, the boolean case should be removed or raise an error.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
api/oas-generator/src/oas_generator/renderer/templates/models/ledger_state_delta.py.j2
Outdated
Show resolved
Hide resolved
api/oas-generator/src/oas_generator/renderer/templates/models/ledger_state_delta.py.j2
Outdated
Show resolved
Hide resolved
5be11b0 to
f355782
Compare
…s in block model Improves msgpack decoding in Algod, Indexer and KMD clients by handling byte keys and values. This prevents decoding errors when encountering non-UTF-8 byte sequences. Additionally, adds decoding for boolean fields in block models to correctly interpret raw values as booleans. This addresses issues with inconsistent data representation.
f355782 to
a1fa8c7
Compare
Proposed Changes
injected only when operations require them. (syncing against latest oas spec refinements from @neilcampbell on ts repo).
with js-algorand-sdk behavior.
decode_bytes_map_key fallback for msgpack maps.