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
fix: multi-call contract operations in tx summary #3710
base: master
Are you sure you want to change the base?
fix: multi-call contract operations in tx summary #3710
Changes from all commits
ae3a6ab
cfddd50
40a889e
e416001
4201b04
c02ff6d
fcd0474
bae9e72
7e11e8f
a96ce82
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.
IIUC, judging by the answers to your questions, we can't rely on max inputs, can't we?
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.
No we can't. But I think there is value as for a tx summary created from a tx response, where it is likely that the max inputs is the same, we will get a more complete summary. It is also failing gracefully (empty array for operations) should the offset be incorrect.
For legacy transactions where it is possible that submitted max inputs != current max inputs, I've got an idea whereby we could query the block header that the tx was included in, and get the consensus param version and check that against the one we want to use for the calculation. This would need another request though. Or we ask them to include that in the transaction status that we have already, that gives us the block ID.
Additionally, there is the tracer but that will also require an additional request.
Going to dump these thoughts on a follow-up issue.
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.
Should we postpone this feature until it is 100% reliable (via the tracer or similar)?
IIRC, one of the use cases for this was to show stuff on the Explorer, correct?
Which, given the circumstances, will not happen anyway.
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.
Max inputs has not changed since we launched main-net. Right now, if we merged this feature, the explorer could display all contract operations for all transactions without making any additional requests.
Right now, we don't have any call operations in the tx summary. If max inputs changes, this feature mirrors that outcome for legacy txs. So IMO there is little downside vs where we are right now.
I'll leave it at that, I think there's value, but I'll leave it to you and the team if there isn't agreement and I'll convert to draft 👍🏻
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.
My point is that there's no guarantee it won't change, right? This is the same caveat from @Torres-ssf's presentation on why we can't hardcode the
maxInputs
value and save one extra request. The same rationale was applied to the RPC Consistency pull request, accounting for variations and "what if it changes" cases.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.
There is no guarantee.
I'll get an issue written up to investigate/introduce tracing as that'll likely be the most robust, and will close this PR with that issue.