[explorer/puller] feat: sync Symbol confirmed transactions and persist mosaic/address rows#2381
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #2381 +/- ##
==========================================
- Coverage 97.01% 95.38% -1.63%
==========================================
Files 276 631 +355
Lines 21314 49344 +28030
Branches 206 1362 +1156
==========================================
+ Hits 20678 47068 +26390
- Misses 630 2226 +1596
- Partials 6 50 +44
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| {'address': bytes.fromhex(TARGET_ADDRESS), 'role': 'target'} | ||
| ]) | ||
|
|
||
| def test_create_transaction_row_leaves_scalar_target_address_empty_for_list_target_type(self): |
There was a problem hiding this comment.
can you explain what you're testing in this test?
There was a problem hiding this comment.
target_address is a scalar column only populated for transaction types with a single target (ACCOUNT_METADATA/MOSAIC_METADATA/NAMESPACE_METADATA/MOSAIC_ADDRESS_RESTRICTION). ACCOUNT_ADDRESS_RESTRICTION has a list of targets (restrictionAdditions/restrictionDeletions), which go into symbol_transaction_addresses with role='target' instead — so this test asserts the scalar column is intentionally left NULL for that type, to make sure we don't accidentally pick one arbitrary address out of the list for it.
There was a problem hiding this comment.
maybe rename the test or add comment so this is clearer
There was a problem hiding this comment.
Added a comment explaining the intent directly on the test — thanks for the suggestion.
| {'address': bytes.fromhex(TARGET_ADDRESS), 'role': 'cosignatory'}, | ||
| {'address': bytes.fromhex(ALIAS_ADDRESS), 'role': 'cosignatory'} |
There was a problem hiding this comment.
i am not sure if same role makes sense for both cosigner add/delete and when cosigner is being used as signer? @cryptoBeliever what is your opinion?
| 'role': 'recipient' | ||
| }) | ||
|
|
||
| if transaction_type in ( |
There was a problem hiding this comment.
one more general comment about structure of this file intended more as informational. feel free to incorporate or not.
the way you split up the file, adding support for a transaction requires changes in multiple places (e.g. for extracting addresses and mosaics) in separate functions and additional branches.
normally, it's a good practice to minimize the number of branches (more for readability and maintenance than performance nowadays given hardware advancements). an alternative design would be to have a small class / tuple with lambdas for each transaction type. instances could live in a dict and there would be one for each transaction that would be retrieved and used for normalization.
for example, something roughly like this
TransactionHandler = namedtuple('TransactionHandler', ['extract_address', 'extract_mosaics'])
def create_transfer_handler():
def extract_address(self, transaction):
return [{
'address': _bytes_from_transaction_field(transaction, 'recipientAddress'),
'role': 'recipient'
}]
def extract_mosaics(self, transaction):
return [
{
'mosaic_id': mosaic['id'],
'amount': int(mosaic['amount']),
'role': 'transfer',
'position': position
}
for position, mosaic in enumerate(transaction.get('mosaics', []))
]
return TransactionHandler(extract_address, extract_mosaics)
TRANSACTION_HANDLERS = {
TRANSFER.value(): create_transfer_handler()
# ...
}again, you don't have to change to this since what you have is fine, but i just wanted to call out a different architecture.
There was a problem hiding this comment.
Thanks for the pointer — makes sense as the codebase grows, but I'll leave the current per-field-branch structure for this phase and revisit if it gets unwieldy.
…t mosaic/address rows
…ansactions Unconfirmed/partial transactions are permanently served from a short-TTL node cache and never persisted to symbol_transactions (this puller only syncs /transactions/confirmed), so group can only ever hold 'confirmed' and has no filter index. There is no reason to keep it as a column; the REST layer can emit "group": "confirmed" as a literal.
…and SQL cascades - Extract camel_case_enum_name/timestamp_from_network_value/label_for_type/ address_from_public_key into format.py, fixing actual duplication between Block.py and Transaction.py (identical type-label lookup, timestamp conversion, and public-key-to-address derivation logic) - Consolidate the repeated DELETE cascade in SymbolDatabase.py into _delete_blocks_and_transactions_from_height - Move duplicated SIGNER_PUBLIC_KEY/SIGNER_ADDRESS/RECIPIENT_ADDRESS/ BENEFICIARY_ADDRESS test constants into tests/test/SymbolTestConstants.py
…AGE_SIZE into MAX_PAGE_SIZE Match the naming symbol-receipts-puller already uses for the same kind of constant, so this line doesn't conflict whichever of the two sibling branches merges second.
…y instead of relying on KeyError
… dict comprehension
…air-rollback assertions
daoka
force-pushed
the
explorer/backend/symbol-transactions-puller
branch
from
931ec8c to
38919e7
Compare
… instead of a combined total
… truncating unmarked messages
2 participants
Summary
symbol_transactions/symbol_transaction_mosaics/symbol_transaction_addressestables andpuller/model/symbol/Transaction.pyto persist confirmed transactions from Symbol Node's/transactions/confirmedendpoint, covering both top-level transactions identified byhashand embedded transactions inside aggregates identified byaggregate_hash+embedded_index.recipient_address/target_addresson the transaction row itself, exist specifically because account/mosaic history search needs to find a transaction by an address or mosaic id it never explicitly names as "the" signer. A plain column or join target is required for that to be indexable.Every other type-specific field, such as
restrictionAdditionsfor an account address restriction ornewRestrictionValuefor a mosaic global restriction, has no such search requirement. It only needs to round-trip for detail-view rendering. Rather than adding a one-off column per field per transaction type, those fields are kept in abodyJSON column: every field of the raw transaction payload except the ones already promoted to their own column, namely signer public key, max fee, deadline, size, type, and message.effective_feefrom the block's fee multiplier usingLEAST(max_fee, size * fee_multiplier), and assign a monotoniclist_sequenceto top-level transactions so the default transaction list can paginate without a SQLOFFSET.type_name,message_type, and mosaic/addressroleare Postgres enum columns rather than freeform text.sizeis a plain integer column, notbigint, since transaction sizes fit comfortably within that range.Note on base branchThis PR targetsexplorer/backend/symbol-block-model-refactorinstead ofdevbecause it depends on that branch's block-row/format-helper extraction (puller/model/symbol/Block.py,puller/model/symbol/format.py), which hasn't merged todevyet (see #2379 ). Once that PR merges, GitHub will offer to retarget this PR's base todevautomatically.