fix: multi-call contract operations in tx summary #3710
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
danielbate
requested review from
digorithm,
arboleya,
Torres-ssf,
nedsalk and
petertonysmith94
as code owners
…tract-ops-multicall
|
Coverage Report:
Changed Files:
|
| const callScriptBaseOffset = | ||
| SCRIPT_FIXED_SIZE + | ||
| calculateScriptVariableSize(callReceiptsForContract) + | ||
| calculateVmTxMemory({ maxInputs: maxInputs.toNumber() }); |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
Release notes
In this release, we:
Summary
Builds on the work in #3704 but makes correct usage of the
param1andparam2contract call receipt properties by using them in conjunction with a calculated script data size, enabling multi-call contract operations.Checklist