HIP-1056 Add block file transformer #10147
Conversation
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10147 +/- ##
==========================================
Coverage 92.30% 92.31%
- Complexity 7821 7840 +19
==========================================
Files 947 951 +4
Lines 32847 32951 +104
Branches 4151 4155 +4
==========================================
+ Hits 30321 30420 +99
- Misses 1550 1555 +5
Partials 976 976 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
| private final PersistProperties persistProperties = new PersistProperties(); | ||
| private final Predicate<EntityId> entityTransactionPredicate = persistProperties::shouldPersistEntityTransaction; | ||
| private final Predicate<EntityId> contractTransactionPredicate = e -> persistProperties.isContractTransaction(); |
There was a problem hiding this comment.
why do we create a PersistProperties object here? In addition, what to persist is in the scope of parser, not downloader.
There was a problem hiding this comment.
An artifact from comparing normal record files to transformed record files. Thanks it is not needed, removed.
| .softwareVersionMajor(major) | ||
| .softwareVersionMinor(minor) | ||
| .softwareVersionPatch(patch) |
There was a problem hiding this comment.
software version is not hapi version, there is a separate software version field in block header.
There was a problem hiding this comment.
Thanks I have corrected it.
| .bytes(blockFile.getBytes()) | ||
| .consensusEnd(blockFile.getConsensusEnd()) | ||
| .consensusStart(blockFile.getConsensusStart()) | ||
| .count((long) blockItems.size()) |
There was a problem hiding this comment.
| .count((long) blockItems.size()) | |
| .count(blockFile.count()) |
| .index(blockHeader.getNumber()) | ||
| .name(blockFile.getName()) | ||
| .nodeId(blockFile.getNodeId()) | ||
| .previousHash(blockHeader.getPreviousBlockHash().toStringUtf8()) |
There was a problem hiding this comment.
| .previousHash(blockHeader.getPreviousBlockHash().toStringUtf8()) | |
| .previousHash(blockFile.getPreviousHash()) |
There was a problem hiding this comment.
Thanks I have fixed it.
| .contractTransactionPredicate(contractTransactionPredicate) | ||
| .entityTransactionPredicate(entityTransactionPredicate) |
There was a problem hiding this comment.
the predicates are set and used in parser, leave them unset here.
There was a problem hiding this comment.
Sounds good, removed.
| return recordItems; | ||
| } | ||
|
|
||
| @SneakyThrows |
There was a problem hiding this comment.
don't think it's good practice to SneakyThrows here, should limit its usage to test only code
There was a problem hiding this comment.
Got it, I have removed it.
| // Note on TransactionHash: | ||
| // There is an Open Issue in HIP 1056 whether the transaction hash will be included in the block stream | ||
| // or if it will need to be calculated by block stream consumers. |
There was a problem hiding this comment.
is that still an open issue? transaction hash is straightforward to calculate and the intention is to save bytes. my recollection is whether or not to do the same for ethereum transaction hash is undecided.
There was a problem hiding this comment.
Thanks, updated this to include calculating the transaction hash.
| public TransactionRecord getTransactionRecord(BlockItem blockItem, TransactionBody transactionBody) { | ||
| var transactionResult = blockItem.transactionResult(); | ||
| var receiptBuilder = TransactionReceipt.newBuilder().setStatus(transactionResult.getStatus()); | ||
| var transactionRecordBuilder = TransactionRecord.newBuilder() |
There was a problem hiding this comment.
add these
- paid_staking_rewards
- parent_consensus_timestamp
- schedule_ref
There was a problem hiding this comment.
Thanks, these are now present
| // or if it will need to be calculated by block stream consumers. | ||
| .setReceipt(receiptBuilder); | ||
|
|
||
| updateTransactionRecord(blockItem.transactionOutput(), transactionRecordBuilder); |
There was a problem hiding this comment.
state changes? should pass the whole blockItem
There was a problem hiding this comment.
Sounds good I have updated this.
| import org.junit.jupiter.params.provider.EnumSource; | ||
|
|
||
| @RequiredArgsConstructor | ||
| public class BlockFileTransformerTest extends ImporterIntegrationTest { |
There was a problem hiding this comment.
why is it an integration test?
There was a problem hiding this comment.
It is using the domainBuilder from the ImporterIntegrationTest.
Signed-off-by: Edwin Greene <[email protected]>
Signed-off-by: Edwin Greene <[email protected]>
|
Description:
Adds the block file transformer for the cryptotransfer transaction type.
Related issue(s):
Fixes #9896
Notes for reviewer:
Checklist