Skip to content

Conversation

Quexington
Copy link
Contributor

@Quexington Quexington commented Jul 18, 2025

With the addition of dicts to streamable, we can finally do this field as it was likely intended to be made. This saves us a lot of headache in tx record creation and removes one aspect of the "convenience" version that we have of transaction records.

The backwards compatibility here is a bit nuanced:

  1. The JSON serialization is obviously a change, but only for the endpoints that return the non-user-friendly version of transaction records. I haven't done an exact survey of these endpoints, but they are certainly in the minority and if we haven't changed them to the user-friendly versions by now, it's likely that nobody uses them the way where this change would affect them. In my evaluation, this should not be a concern.
  2. There was actually a bug (at least I hope it was unintentional) in the user-friendly serialization of the memos dictionary where it would always have a single value per key instead of a list of values and would only show the last memo. This was due to a comprehension that iterated over the memos but just repeatedly assigned them to the same key. I have "accidentally" fixed this by deleting that custom serialization logic, but this means that the memos in the user-friendly form are a different format now. Given that no one noticed this before I take this as evidence that the change here shouldn't matter all that much.
  3. The byte serialization is actually compatible! This is because the streamable dict functionality serializes by borrowing the logic from lists and tuples, which is how this field is currently represented in python. I have confirmed that I upgraded to this code and ran a wallet and got transactions successfully.

@Quexington Quexington requested a review from a team as a code owner July 18, 2025 09:08
@Quexington Quexington added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Jul 18, 2025
@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Jul 18, 2025
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Jul 21, 2025
@Quexington Quexington requested a review from altendky July 21, 2025 09:42
Copy link

Pull Request Test Coverage Report for Build 16413477088

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 12 of 12 (100.0%) changed or added relevant lines in 6 files are covered.
  • 20 unchanged lines in 10 files lost coverage.
  • Overall coverage decreased (-0.004%) to 91.291%

Files with Coverage Reduction New Missed Lines %
chia/daemon/client.py 1 74.16%
chia/daemon/keychain_proxy.py 1 73.24%
chia/farmer/farmer.py 1 73.28%
chia/full_node/pending_tx_cache.py 1 96.55%
chia/server/node_discovery.py 1 81.93%
chia/full_node/full_node_api.py 2 85.45%
chia/wallet/transaction_record.py 2 95.69%
chia/wallet/wallet_request_types.py 2 99.37%
chia/server/server.py 4 82.51%
chia/full_node/full_node.py 5 88.05%
Totals Coverage Status
Change from base Build 16375210539: -0.004%
Covered Lines: 101890
Relevant Lines: 111489

💛 - Coveralls

@Starttoaster Starttoaster merged commit 55165b1 into main Jul 23, 2025
522 of 525 checks passed
@Starttoaster Starttoaster deleted the quex.use_dicts_for_tx_record_memos branch July 23, 2025 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changed Required label for PR that categorizes merge commit message as "Changed" for changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants