Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
HIP-1056 Account BlockItem to RecordItem transformers #10249
base: main
Are you sure you want to change the base?
HIP-1056 Account BlockItem to RecordItem transformers #10249
Changes from 6 commits
cce3dbd
311084d
9094268
88b9dde
cab6e84
c69c1b7
2df4bfe
c15fc81
ccae223
2ab0eeb
4505520
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We'll want to update this with the new
blockItem.successful()
method from #10248 once it's merged.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.
I will update it once the PR is merged.
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.
The key issue here is any account with state changes will be here, with exact same state id, change case, key case, and value case.
It's very common for any transaction having at least the following account map update in state changes
There isn't a way for us to confidently identify the new account. Using the key in the transaction body can work in most cases however the network allows many accounts to use the same key so it's not a 100% reliable method.
We may have to ask for a change to add the new account id to
CreateAccountOutput
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.
@edwin-greene Please chase this down with the relevant teams
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.
I agree that this is a miss in the protobuf definitions.
Suggested resolution:
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.
To be complete.
This is, I believe, specific to account. In most cases the state changes (which immediately follow the triggering transaction) are more than sufficient to identify the created entity.
Unfortunately, as noted above, Account state changes follow every transaction, and it is unusually difficult to separate the create from other account updates; particularly if an account is created as a result of an EVM transaction.
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.
receipt.account_id
shouldn't be set for crypto delete transactionThere 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.
Updated
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.
weird enough,
receipt.account_id
for crypto update transaction is set in record stream files.again, the logic shares the same problem in the comment for crypto create transaction.
Think we can choose not to set it at all, or set it to the value from crypto update transaction body
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.
Temporarily removing the updateTransactionRecord method until we identify a better solution.