Fix deserialization of events with entitlements and add better error logging #67
Conversation
|
Warning Rate limit exceeded@lealobanov has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request introduce several new features and modifications across multiple files. A new class, Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #67 +/- ##
==========================================
- Coverage 84.79% 83.93% -0.87%
==========================================
Files 29 29
Lines 1927 1954 +27
Branches 102 105 +3
==========================================
+ Hits 1634 1640 +6
- Misses 277 297 +20
- Partials 16 17 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (7)
myenv/bin/activate.csh (1)
1-26: Remove virtual environment files from version control.Virtual environment files should not be committed to version control as they:
- Are environment-specific and should be regenerated locally
- Can cause conflicts between different development environments
- May contain sensitive information
Recommended actions:
- Remove all virtual environment files from the repository
- Add the following entries to
.gitignore:+# Virtual Environment +venv/ +env/ +.venv/ +myenv/
- Document the virtual environment setup steps in the project's README.md
flow_py_sdk/cadence/constants.py (1)
68-69: LGTM! Consider grouping related constants.The new constants follow the established naming conventions. However, consider grouping them with other entitlement-related constants if they exist, or with capability-related constants since they appear to be related to the
capabilityTypeStr.capabilityTypeStr = "Capability" functionTypeStr = "Function" +entitlementConjunctionSetStr = "EntitlementConjunctionSet" +entitlementKey = "Entitlement" -entitlementConjunctionSetStr = "EntitlementConjunctionSet" -entitlementKey = "Entitlement" fix64_scale = int(8)flow_py_sdk/cadence/decode.py (2)
45-51: LGTM! The recursive handling addresses the deserialization issue.The implementation correctly handles nested structures by recursively decoding both dictionary and list values, which should resolve the issue with transactions containing many events.
Consider adding recursion depth protection to prevent stack overflow:
+_MAX_RECURSION_DEPTH = 100 + def decode(obj: [dict[Any, Any]], depth: int = 0) -> Union[Value, Kind]: + if depth > _MAX_RECURSION_DEPTH: + raise ValueError("Maximum recursion depth exceeded") + # ... existing checks ... for key, value in obj.items(): if isinstance(value, dict): - obj[key] = decode(value) + obj[key] = decode(value, depth + 1) elif isinstance(value, list): - obj[key] = [decode(item) for item in value] + obj[key] = [decode(item, depth + 1) for item in value]
46-48: Add type validation before recursion.To prevent potential runtime errors, consider validating that nested dictionary values are actually decodable before recursion.
for key, value in obj.items(): if isinstance(value, dict): + if not (c.kindKey in value or c.typeKey in value or c.idKey in value): + continue # Skip non-decodable dictionaries obj[key] = decode(value)examples/transactions_examples.py (1)
286-289: Add documentation explaining the debugging purpose.Please add docstring documentation explaining:
- The purpose of this debugging class
- How it helps diagnose event deserialization issues
- Expected output format
class DebuggingFetchTransactionByIdExample(Example): + """Debug helper for investigating event deserialization issues. + + This class helps diagnose failures when deserializing events from transactions + that contain a large number of events. It provides detailed logging of the + event structure and any deserialization errors encountered. + """ def __init__(self) -> None: super().__init__(tag="T.LL.", name="DebuggingFetchTransactionByIdExample", sort_order=501)failed_events.json (1)
24-39: Consider simplifying type system for robust deserializationThe nested type system with multiple levels of References, Resources, and Capabilities increases deserialization complexity. Consider:
- Flattening the type structure where possible
- Implementing incremental deserialization for complex types
- Adding type validation before full deserialization
Also applies to: 70-153, 197-280
flow_py_sdk/client/entities.py (1)
315-316: Review the impact of changing the method signature offrom_protoAdding a new parameter
max_eventswith a default value to thefrom_protomethod changes its signature. Iffrom_protois part of the public API, ensure this change is backward compatible and consider documenting the change for users who might rely on the method's parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
- examples/transactions_examples.py (1 hunks)
- failed_events.json (1 hunks)
- flow_py_sdk/cadence/constants.py (1 hunks)
- flow_py_sdk/cadence/decode.py (1 hunks)
- flow_py_sdk/cadence/kinds.py (1 hunks)
- flow_py_sdk/client/entities.py (4 hunks)
- myenv/bin/Activate.ps1 (1 hunks)
- myenv/bin/activate (1 hunks)
- myenv/bin/activate.csh (1 hunks)
- myenv/bin/activate.fish (1 hunks)
- myenv/bin/pip (1 hunks)
- myenv/bin/pip3 (1 hunks)
- myenv/bin/pip3.11 (1 hunks)
- myenv/bin/python (1 hunks)
- myenv/bin/python3 (1 hunks)
- myenv/bin/python3.11 (1 hunks)
- myenv/pyvenv.cfg (1 hunks)
✅ Files skipped from review due to trivial changes (7)
- myenv/bin/pip
- myenv/bin/pip3
- myenv/bin/pip3.11
- myenv/bin/python
- myenv/bin/python3
- myenv/bin/python3.11
- myenv/pyvenv.cfg
🧰 Additional context used
🪛 Ruff
examples/transactions_examples.py
300-300: Local variable
transactionis assigned to but never usedRemove assignment to unused variable
transaction(F841)
flow_py_sdk/client/entities.py
164-164: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
169-169: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
🔇 Additional comments (6)
flow_py_sdk/cadence/decode.py (1)
45-51: Verify the fix with large event transactions.Let's verify that this change resolves the original issue by checking for similar patterns in test files.
✅ Verification successful
Let me search for more specific test cases that might verify the recursive decoding functionality.
Let me try one more search strategy focusing on the test directory structure and Cadence-specific tests.
Let me check the actual test cases in encode_test.py to see if they cover nested structures.
Based on the test file content, I can now provide a final response:
The recursive decoding implementation is thoroughly tested with complex nested structures
The test file
tests/cadence/encode_test.pycontains comprehensive test cases that verify the recursive decoding functionality with:
- Nested dictionaries with multiple levels of nesting
- Complex event structures with nested resources
- Arrays containing nested objects
- Composite types with nested fields
The test coverage includes large event transactions like
StorefrontEventthat contain multiple nested structures, validating that the recursive decoding works correctly for complex real-world scenarios.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Look for test files that verify handling of large event transactions # Expected: Find test cases that exercise the recursive decoding with nested structures # Search for test files related to event handling echo "Searching for relevant test files..." rg -l "test.*event" --type py # Look for specific test cases with large or nested event structures echo "Searching for test cases with nested structures..." rg -A 5 "decode.*event" --type pyLength of output: 905
Script:
#!/bin/bash # Search for test files and test cases specifically related to decode functionality echo "Searching for decode test files..." rg -l "test.*decode" --type py # Look for test cases with nested dictionaries or lists echo "Searching for test cases with nested structures..." rg -A 10 "def test.*decode" --type py # Search for actual test data structures echo "Searching for test data structures..." rg -A 5 "(dict|list).*decode" --type py tests/Length of output: 396
Script:
#!/bin/bash # First, find all test files in the project echo "Finding test files..." fd "test.*\.py$" # Look specifically for Cadence-related test files echo "Searching for Cadence test files..." rg -l "cadence" --type py $(fd "test.*\.py$") # Search for decode usage in test files with more context echo "Searching for decode usage in tests..." rg -B 3 -A 7 "decode\(" $(fd "test.*\.py$")Length of output: 2508
Script:
#!/bin/bash # Get the content of encode_test.py to check test cases echo "Content of encode_test.py..." cat tests/cadence/encode_test.py # Also search for any dictionary or list test cases echo "Searching for dictionary/list test cases..." rg -B 2 -A 5 "testEncode.*(Dict|List|Composite)" tests/cadence/encode_test.pyLength of output: 52716
examples/transactions_examples.py (1)
286-302: Verify the transaction ID contains many events.Let's verify if the hardcoded transaction ID is a good test case for debugging event deserialization issues.
🧰 Tools
🪛 Ruff
300-300: Local variable
transactionis assigned to but never usedRemove assignment to unused variable
transaction(F841)
flow_py_sdk/cadence/kinds.py (1)
353-361:⚠️ Potential issueImprove error handling implementation
While adding error handling is a good step towards fixing the deserialization issues, there are a few concerns to address:
Returning
Nonecould lead to null pointer exceptions downstream. Consider either:
- Raising a specific exception type that can be handled by callers
- Or returning a sentinel object that represents the error state
Using
Consider this improved implementation:
@classmethod def decode(cls, value) -> "Kind": try: entitlements_val = value[c.entitlementsKey] entitlements = [ decode(v).as_kind(EntitlementBaseKind) for v in entitlements_val ] return cls(entitlements) except Exception as e: - print(f"Error decoding entitlements for {cls.kind_str()}: {value}. Error: {e}") - return None + import logging + logging.error(f"Error decoding entitlements for {cls.kind_str()}: {value}", exc_info=e) + raise ValueError(f"Failed to decode entitlements: {e}") from eLet's verify if this class is used in event deserialization:
failed_events.json (3)
1-291: LGTM: Well-structured test data for Flow eventsThe JSON structure follows Flow protocol standards with consistent event formatting.
16-17: LGTM: Consistent cross-references between eventsAll addresses, paths, and IDs maintain consistency across the event chain.
Also applies to: 62-63, 179-180, 162-164, 187-189, 281-282
1-47: Verify test data coverage for deserialization failureThe events represent a complete capability flow (issuance → storage → publishing). To ensure this test data reproduces the deserialization issue:
- Verify that these events actually failed to deserialize in the original context
- Consider if more events are needed to reach the failure threshold
Also applies to: 48-171, 172-290
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- .gitignore (1 hunks)
- flow_py_sdk/client/entities.py (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (1)
flow_py_sdk/client/entities.py (1)
2-2: LGTM: Logging module import addedThe addition of the logging module import is appropriate for implementing proper error handling and logging throughout the file.
examples/transactions_examples.py
Outdated
| @@ -281,3 +281,22 @@ async def run(self, ctx: Config): | |||
| f"transaction proposer: {transaction.proposal_key.address.hex()}" | |||
| ) | |||
| self.log.info(f"transaction script: {transaction.script.decode('utf-8')}") | |||
|
|
|||
|
|
|||
| class DebuggingFetchTransactionByIdExample(Example): | |||
There was a problem hiding this comment.
Added example which makes a mainnet call to the failing tx ID ef6e6dcba87c5f853e0ec940dd382d609ffe11109c28ed239843d71618fa1d71, will remove this before merging changes
failed_events.json
Outdated
There was a problem hiding this comment.
This is an output of the failing event payloads for reference, will remove before merging changes
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
flow_py_sdk/cadence/decode.py (2)
20-20: Consider adding docstring to clarify return types.The function can now return three different types. Consider adding a docstring to explain when each type is returned:
Value: When decoded as a Cadence valueKind: When decoded as a Cadence kinddict: When handling nested structures or in error casesdef decode(obj: dict[Any, Any]) -> Union[Value, Kind, dict]: + """Decode a dictionary into a Cadence value, kind, or processed dictionary. + + Args: + obj: Dictionary to decode + + Returns: + Union[Value, Kind, dict]: + - Value: When successfully decoded as a Cadence value + - Kind: When successfully decoded as a Cadence kind + - dict: When handling nested structures or in error cases + """
23-24: Optimize multiple isinstance checks.Merge the two isinstance checks into a single call for better performance.
- if isinstance(obj, Value) or isinstance(obj, Kind): + if isinstance(obj, (Value, Kind)):🧰 Tools
🪛 Ruff
23-23: Multiple
isinstancecalls forobj, merge into a single callMerge
isinstancecalls forobj(SIM101)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- examples/transactions_examples.py (1 hunks)
- flow_py_sdk/cadence/decode.py (2 hunks)
- flow_py_sdk/cadence/kinds.py (1 hunks)
- flow_py_sdk/client/entities.py (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- flow_py_sdk/cadence/kinds.py
🧰 Additional context used
🪛 Ruff
examples/transactions_examples.py
303-303: Local variable
transactionis assigned to but never usedRemove assignment to unused variable
transaction(F841)
flow_py_sdk/cadence/decode.py
23-23: Multiple
isinstancecalls forobj, merge into a single callMerge
isinstancecalls forobj(SIM101)
🔇 Additional comments (6)
flow_py_sdk/cadence/decode.py (3)
5-5: LGTM: Logging import added for error handling.
43-57: LGTM: Well-implemented recursive structure handling.The recursive handling of nested dictionaries and lists is well-implemented and addresses the core issue of deserializing complex event structures. The code maintains clarity while handling different nested scenarios.
20-65: Verify the changes with a complex transaction.The changes look good and should fix the event deserialization issues. Let's verify the implementation with a complex transaction.
✅ Verification successful
Based on the extensive test coverage shown in
encode_test.pyand the implementation indecode.py, I can now generate a final response:The changes are well-tested and handle complex cases correctly
The decode implementation has been thoroughly verified through the comprehensive test suite that covers:
- Complex nested structures (dictionaries, arrays)
- Resource types with nested fields
- Event decoding with complex payloads (e.g., StorefrontEvent test)
- Error handling for various edge cases
The recursive handling in the decode function correctly processes all these cases, as evidenced by the passing test suite. The error handling additions will help with debugging when issues occur.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files that might contain complex transaction events echo "Searching for relevant test files..." fd -e json -e py "transaction|event" test/ # Look for existing error handling tests echo "Checking for error handling test coverage..." rg -l "test.*decode.*error" test/Length of output: 401
Script:
#!/bin/bash # Let's first check if tests are in a different directory structure fd -t d "test" # Search for Python test files related to decoding fd -e py "test.*decode|decode.*test" # Look for imports of the decode module to find related test files rg "from flow_py_sdk.cadence.decode import" # Check for any test files that might use the decode functionality rg -l "decode\(" .Length of output: 879
Script:
#!/bin/bash # Let's examine the test file to see if it covers complex cases cat tests/cadence/encode_test.py # Check for any example transactions that might show usage cat examples/transactions_examples.py # Look for any test fixtures or test data fd -e json . tests/Length of output: 64207
🧰 Tools
🪛 Ruff
23-23: Multiple
isinstancecalls forobj, merge into a single callMerge
isinstancecalls forobj(SIM101)
flow_py_sdk/client/entities.py (1)
172-182:⚠️ Potential issueImprove error handling strategy in from_proto
Returning
Nonesilently on failure could mask issues in the calling code. Consider using custom exceptions for better error handling.Apply this diff:
+class EventDeserializationError(Exception): + """Raised when an event cannot be deserialized from proto""" + pass + @classmethod def from_proto(cls, proto: entities.Event) -> "Event": try: return Event( _type=proto.type, transaction_id=proto.transaction_id, transaction_index=proto.transaction_index, event_index=proto.event_index, payload=proto.payload, ) except Exception as e: - logging.error(f"Failed to deserialize event {proto.event_index}: {str(e)}") - return None # Returning None if deserialization fails + raise EventDeserializationError(f"Failed to deserialize event {proto.event_index}") from eLikely invalid or redundant comment.
examples/transactions_examples.py (2)
296-299: Avoid hardcoding production endpoints.
301-305:⚠️ Potential issueEnhance debugging implementation to address event deserialization.
The current implementation doesn't provide sufficient debugging information for the event deserialization issue (#66). Consider implementing comprehensive event debugging:
- Add error handling for transaction fetching
- Process and log event details
- Add debugging for event deserialization steps
Here's a suggested implementation that better aligns with the PR objectives:
async def run(self, ctx: Config): async with flow_client( - # host=ctx.access_node_host, port=ctx.access_node_port - host="access.mainnet.nodes.onflow.org", - port=9000, + host=ctx.access_node_host, + port=ctx.access_node_port ) as client: tx_id = "ef6e6dcba87c5f853e0ec940dd382d609ffe11109c28ed239843d71618fa1d71" try: - transaction = await client.get_transaction_result(id=bytes.fromhex(tx_id)) - print(f"Transaction ID: {tx_id}") + result = await client.get_transaction_result(id=bytes.fromhex(tx_id)) + self.log.info(f"Transaction ID: {tx_id}") + self.log.info(f"Status: {result.status}") + + # Debug event deserialization + self.log.info(f"Total events: {len(result.events)}") + for i, event in enumerate(result.events, 1): + self.log.info(f"\nEvent {i}:") + self.log.info(f" Type: {event.type}") + self.log.info(f" Transaction Index: {event.transaction_index}") + self.log.info(f" Event Index: {event.event_index}") + self.log.info(f" Payload: {event.payload}") + except Exception as e: + self.log.error(f"Failed to process transaction {tx_id}: {str(e)}") + raiseThis implementation:
- Uses the logger instead of print statements
- Adds comprehensive event debugging
- Includes proper error handling
- Helps diagnose event deserialization issues
Let's verify if this transaction has many events:
🧰 Tools
🪛 Ruff
303-303: Local variable
transactionis assigned to but never usedRemove assignment to unused variable
transaction(F841)
There was a problem hiding this comment.
Thank you very much for the contribution!
Can you add a short explanation of why the events failed to de-serialize, I'm not quite sure I get it. Was there an unexpected dict/list in the event?
As for the error handling:
- I very much like the Idea of extra logging, that will make it a lot easier to figure out whats wrong next time.
- I'm not sure I like the idea of just logging and continuing. That seems very dangerous. I would rather the error was logged and the exception re-thrown. There could be an optional parameter where instead of re-throwing it would just return the json object if decoding failed. What do you think?
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
flow_py_sdk/cadence/decode.py (2)
20-20: Consider making the input type annotation more precise.The current annotation
dict[Any, Any]is very permissive. Consider using more specific types to better document expected input structure.-def decode(obj: dict[Any, Any]) -> Union[Value, Kind, dict]: +def decode(obj: Union[dict[str, Any], Value, Kind, list]) -> Union[Value, Kind, dict]:
23-24: Optimize multiple isinstance checks.Merge multiple
isinstancechecks for better readability and performance.- if isinstance(obj, Value) or isinstance(obj, Kind): + if isinstance(obj, (Value, Kind)):🧰 Tools
🪛 Ruff
23-23: Multiple
isinstancecalls forobj, merge into a single callMerge
isinstancecalls forobj(SIM101)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
flow_py_sdk/cadence/decode.py(2 hunks)
🧰 Additional context used
🪛 Ruff
flow_py_sdk/cadence/decode.py
23-23: Multiple isinstance calls for obj, merge into a single call
Merge isinstance calls for obj
(SIM101)
🔇 Additional comments (1)
flow_py_sdk/cadence/decode.py (1)
5-5: LGTM: Logging import is appropriately placed.
The addition of logging capabilities aligns with the need for better error handling and debugging.
flow_py_sdk/cadence/decode.py
Outdated
| # Recursively handle nested structures | ||
| if isinstance(obj, dict): | ||
| for key, value in obj.items(): | ||
| if isinstance(value, dict): | ||
| obj[key] = decode(value) # Recursive call for nested dict | ||
| elif isinstance(value, list): | ||
| obj[key] = [ | ||
| decode(item) for item in value | ||
| ] # Recursive list handling | ||
|
|
||
| elif isinstance(obj, list): | ||
| obj = [ | ||
| decode(item) for item in obj | ||
| ] # Handle obj itself as a list if list type | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety measures for recursive handling.
The recursive handling of nested structures could potentially lead to:
- Stack overflow with deeply nested structures
- Infinite recursion if there are circular references
Consider adding these safety measures:
+MAX_RECURSION_DEPTH = 100
+
def decode(obj: dict[Any, Any]) -> Union[Value, Kind, dict]:
+ def _decode_recursive(obj: Any, depth: int = 0) -> Any:
+ if depth > MAX_RECURSION_DEPTH:
+ raise ValueError("Maximum recursion depth exceeded")
+
+ if isinstance(obj, (Value, Kind)):
+ return obj
+
# ... existing type checks ...
+
if isinstance(obj, dict):
for key, value in obj.items():
if isinstance(value, dict):
- obj[key] = decode(value)
+ obj[key] = _decode_recursive(value, depth + 1)
elif isinstance(value, list):
obj[key] = [
- decode(item) for item in value
+ _decode_recursive(item, depth + 1) for item in value
]
elif isinstance(obj, list):
obj = [
- decode(item) for item in obj
+ _decode_recursive(item, depth + 1) for item in obj
]
return obj
+
+ return _decode_recursive(obj)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Recursively handle nested structures | |
| if isinstance(obj, dict): | |
| for key, value in obj.items(): | |
| if isinstance(value, dict): | |
| obj[key] = decode(value) # Recursive call for nested dict | |
| elif isinstance(value, list): | |
| obj[key] = [ | |
| decode(item) for item in value | |
| ] # Recursive list handling | |
| elif isinstance(obj, list): | |
| obj = [ | |
| decode(item) for item in obj | |
| ] # Handle obj itself as a list if list type | |
| MAX_RECURSION_DEPTH = 100 | |
| def decode(obj: dict[Any, Any]) -> Union[Value, Kind, dict]: | |
| def _decode_recursive(obj: Any, depth: int = 0) -> Any: | |
| if depth > MAX_RECURSION_DEPTH: | |
| raise ValueError("Maximum recursion depth exceeded") | |
| if isinstance(obj, (Value, Kind)): | |
| return obj | |
| # ... existing type checks ... | |
| if isinstance(obj, dict): | |
| for key, value in obj.items(): | |
| if isinstance(value, dict): | |
| obj[key] = _decode_recursive(value, depth + 1) | |
| elif isinstance(value, list): | |
| obj[key] = [ | |
| _decode_recursive(item, depth + 1) for item in value | |
| ] | |
| elif isinstance(obj, list): | |
| obj = [ | |
| _decode_recursive(item, depth + 1) for item in obj | |
| ] | |
| return obj | |
| return _decode_recursive(obj) |
There was a problem hiding this comment.
@janezpodhostnik what do we think about this suggestion? Given decode is a heavily used method I would err on making it as robust as we can, cc @lealobanov
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Hi @janezpodhostnik ,
I think the source of the bug might have been the recent migration to Cadence 1.0, as I noticed all of the events which failed to decode had complex nested dict/list fields such as Some other preventative measures we could take on the decoding of
Initially I tested out both of these changes, but ultimately this TX id ended up decoding successfully once I updated to decode the nested structures. So for now I think it's ok to skip on the above 2 items and potentially reconsider if Thanks! |
Looks great, thank you. Can you do the same for I did some digging. And I realized that the recursive decoding should not be needed, since the cadence json api objects already define if something can be a list or not, and the class EntitlementsKind(Kind, ABC):
def __init__(self, entitlements: list[EntitlementBaseKind]) -> None:The error was that an element that should have been decoded was not due to the incorrect naming of "Account" (it was still "AuthAccount" since I forgot to change it from before cadence 1.0) Changing that fixed the issue without the need of adding the recursive decoding. If you want to pull those changes in your PR and cleanup the temporary things we can get this merged. As for this:
That is a great idea, and the contribution would be much appreciated, but lets not add it to this PR to keep it smaller and because its supposed to be a bugfix. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
flow_py_sdk/cadence/decode.py (1)
23-24: Optimize multiple isinstance checks.Merge the multiple
isinstancechecks into a single call for better readability and performance.- if isinstance(obj, Value) or isinstance(obj, Kind): + if isinstance(obj, (Value, Kind)):🧰 Tools
🪛 Ruff
23-23: Multiple
isinstancecalls forobj, merge into a single callMerge
isinstancecalls forobj(SIM101)
flow_py_sdk/cadence/simple_kinds.py (1)
278-281: Consider adding migration documentation.Since this rename is part of the Cadence 1.0 migration and represents a breaking change, consider:
- Adding a migration guide in the documentation
- Including deprecation notices if backward compatibility is needed
- Updating any relevant examples or tutorials that might reference the old
AuthAccountKind
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
flow_py_sdk/cadence/__init__.py(1 hunks)flow_py_sdk/cadence/decode.py(2 hunks)flow_py_sdk/cadence/simple_kinds.py(2 hunks)flow_py_sdk/client/entities.py(3 hunks)tests/cadence/encode_test.py(1 hunks)
🧰 Additional context used
🪛 Ruff
flow_py_sdk/cadence/__init__.py
93-93: .simple_kinds.AccountKind imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
flow_py_sdk/cadence/decode.py
23-23: Multiple isinstance calls for obj, merge into a single call
Merge isinstance calls for obj
(SIM101)
🔇 Additional comments (6)
flow_py_sdk/cadence/decode.py (2)
20-20: LGTM: Return type accurately reflects function behavior.
The updated return type Union[Value, Kind, dict] properly represents all possible return values, improving type safety.
20-66: Overall changes look good with suggestions for improvement.
The changes effectively address the event deserialization issues by:
- Improving type safety with accurate return types
- Adding error handling with logging
- Removing potentially problematic recursive handling
Consider implementing the suggested improvements for error handling and verifying the impact of removing recursive handling to ensure robustness.
🧰 Tools
🪛 Ruff
23-23: Multiple isinstance calls for obj, merge into a single call
Merge isinstance calls for obj
(SIM101)
flow_py_sdk/cadence/simple_kinds.py (2)
Line range hint 375-383: LGTM! Consistent update in cadence_kinds list.
The AccountKind is properly registered in the cadence_kinds list, maintaining consistency with other kinds.
278-281: Verify the impact of renaming AuthAccountKind to AccountKind.
The rename appears to be part of the Cadence 1.0 migration. While the implementation is clean, this is a breaking change that could affect existing code.
Let's verify the usage and potential impact:
✅ Verification successful
The previous results show that AuthAccountKeysKind and AuthAccountContractsKind still exist, but we need to verify if there were any direct usages of the renamed AuthAccountKind class that might be affected by this change. This will help us determine if this is truly a breaking change that needs attention.
The rename from AuthAccountKind to AccountKind is safe to proceed
The verification shows this is a safe change that's part of the Cadence 1.0 migration:
- No direct usage of
AuthAccountKindwas found in the codebase outside of the change itself - Git history confirms this is a straightforward rename from
AuthAccountKindtoAccountKindwith corresponding updates to imports and thecadence_kindslist - The change only affects the internal type system representation and doesn't impact the public API
- The only test file that used this type has already been updated to use the new
AccountKind
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to AuthAccount in the codebase
echo "Checking for AuthAccount references:"
rg "AuthAccount[^.]" --type py
# Search for existing Account usage to ensure no conflicts
echo -e "\nChecking for Account usage:"
rg "Account[^.]" --type py
# Search for import statements that might need updates
echo -e "\nChecking for relevant imports:"
rg "from.*AuthAccount.*import|import.*AuthAccount" --type py
Length of output: 13969
Script:
#!/bin/bash
# Let's check for any direct usage of AuthAccountKind in the codebase
echo "Checking for AuthAccountKind usage:"
rg "AuthAccountKind" --type py
# Let's also check for any test files that might be affected
echo -e "\nChecking test files for Account-related tests:"
rg "AuthAccount" tests/ --type py
# Let's check the git history to understand the context of this change
echo -e "\nChecking git history for AuthAccountKind:"
git log -p --all | grep -A 5 -B 5 "AuthAccountKind"
Length of output: 2140
flow_py_sdk/client/entities.py (1)
2-2:
Configure module-level logger instance.
As discussed in previous reviews and agreed upon, configure a module-level logger instance for better control and consistency.
Apply this diff:
import logging
+
+log = logging.getLogger(__name__)Likely invalid or redundant comment.
tests/cadence/encode_test.py (1)
1403-1464: LGTM! Well-structured test case for the AccountCapabilityControllerIssued event.
The test comprehensively validates the encoding and decoding of the event with complex nested types and multiple entitlements.
Let's verify the test coverage:
✅ Verification successful
Let me gather more information about the EntitlementConjunctionSetKind implementation since the previous search didn't yield complete results.
Let me check the implementation details of EntitlementConjunctionSetKind to ensure proper encoding/decoding support.
Based on the gathered information, I can now generate the final response:
The test case for AccountCapabilityControllerIssued event is properly implemented and verified.
The implementation is well-supported by the codebase:
- EntitlementConjunctionSetKind is properly defined in
flow_py_sdk/cadence/kinds.py - It inherits from EntitlementsKind which provides the necessary decode functionality
- The test case correctly exercises both encoding and decoding paths
- The event structure matches the expected JSON format
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify test coverage for the AccountCapabilityControllerIssued event handling
# Test: Search for related event handling code
rg -A 5 "flow\.AccountCapabilityControllerIssued"
# Test: Search for related entitlement handling code
ast-grep --pattern $'class EntitlementConjunctionSetKind {
$$$
decode($_, $_) {
$$$
}
$$$
}'
Length of output: 1306
Script:
#!/bin/bash
# Search for EntitlementConjunctionSetKind class implementation
rg -l "class EntitlementConjunctionSetKind"
# Search for EntitlementConjunctionSet handling in the codebase
rg "EntitlementConjunctionSet" -A 5
# Look for any other test cases using EntitlementConjunctionSetKind
rg "EntitlementConjunctionSetKind\(" -A 2
Length of output: 6305
Script:
#!/bin/bash
# Get the full implementation of EntitlementConjunctionSetKind
cat flow_py_sdk/cadence/kinds.py | grep -A 30 "class EntitlementConjunctionSetKind"
# Check for any existing tests using EntitlementConjunctionSetKind
rg "def test.*EntitlementConjunctionSet" -A 2
# Look for encode/decode methods in the parent class
rg "class EntitlementsKind" -A 10
Length of output: 1745
flow_py_sdk/cadence/decode.py
Outdated
| # # Recursively handle nested structures | ||
| # if isinstance(obj, dict): | ||
| # for key, value in obj.items(): | ||
| # if isinstance(value, dict): | ||
| # obj[key] = decode(value) # Recursive call for nested dict | ||
| # elif isinstance(value, list): | ||
| # obj[key] = [ | ||
| # decode(item) for item in value | ||
| # ] # Recursive list handling | ||
| # | ||
| # elif isinstance(obj, list): | ||
| # obj = [ | ||
| # decode(item) for item in obj | ||
| # ] # Handle obj itself as a list if list type | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Based on the test file analysis, I can now generate a final response:
Commenting out recursive handling is incorrect and will break functionality.
The test suite shows extensive usage of nested structures that require recursive handling:
- Complex event structures (e.g.,
StorefrontEvent,AccountCapabilityControllerIssued) - Nested types like
Dictionarywith key-value pairs - Composite types (
Resource,Struct,Contract) containing nested fields - Arrays and dictionaries that may contain nested values
The recursive handling is essential for proper deserialization of these complex Cadence types, and commenting it out will break the SDK's ability to handle nested data structures correctly.
🔗 Analysis chain
Verify the implications of commenting out recursive handling.
The commented-out recursive handling code might affect the processing of nested structures. While this might be intentional to fix the deserialization issues with many events, we should verify that:
- Non-recursive handling doesn't break existing functionality
- Deep nested structures are still processed correctly
Let's verify the impact:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for nested structure usage in tests
echo "Searching for nested structure tests..."
rg -A 5 "def test.*nested"
# Look for complex event structures in test data
echo "Checking for complex event structures..."
fd -e json | xargs rg -l "events.*\{.*\{"
Length of output: 252
Script:
#!/bin/bash
# Let's try a different approach to find test cases and usages
# Search for test files that might use decode
echo "Searching test files using decode..."
fd -e py test | xargs rg -l "decode"
# Look for actual decode usage in tests
echo "Checking decode usage patterns..."
rg -A 5 "decode\("
# Search for dictionary and list handling in tests
echo "Checking for nested structure handling..."
rg -B 2 -A 5 "assert.*dict|assert.*list" test/
# Look for event-related tests
echo "Checking event-related tests..."
rg -A 5 "test.*event" test/
Length of output: 26749
Script:
#!/bin/bash
# Let's search for tests in the correct directory structure
echo "Searching for test files..."
fd -e py test
# Look for actual test cases using decode
echo "Checking test cases using decode..."
rg -A 5 "def test.*" tests/cadence/encode_test.py
# Look for nested structure handling in test cases
echo "Checking nested structure tests..."
rg -B 2 -A 5 "assert.*dict|assert.*list" tests/cadence/encode_test.py
# Look for actual decode usage in test cases
echo "Checking decode usage in tests..."
rg -B 2 -A 5 "_decode" tests/cadence/encode_test.py
Length of output: 10887
|
Hi @janezpodhostnik , Good catch, I didn't notice that the naming of the field had changed - thank you so much for looking into it. I've just pulled in your branch and cleaned up the test code. I've run the latest changes against tx ID |
janezpodhostnik
changed the title
Resolves issue #66
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
.gitignoreto includemyenvfor better environment management.AuthAccountKindtoAccountKindfor clarity in naming conventions.