-
Notifications
You must be signed in to change notification settings - Fork 970
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
Centralize txn size calculation #4064
Centralize txn size calculation #4064
Conversation
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.
Thanks for this cleanup! I think there are a few more places in the code where we should be using the new getTransactionEnvelopeSize
instead of the old calculation. I see some places in src/herder/test/HerderTests.cpp
, src/transactions/test/InvokeHostFunctionTests.cpp
, and src/transactions/test/SorobanTxTestUtils.cpp
. Any place where we have xdr::xdr_size(txEnvelope)
should probably use the new function for consistency.
if (isSoroban()) | ||
{ | ||
auto txSize = static_cast<int64_t>( |
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.
Why did this move inside the if? Seems cleaner to get the size just once like before.
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 don't use the size in the final else
case (non-soroban, non-useByteLimitInClassic), so I moved the computation into the cases where we are using it. But I agree its cleaner to only have the computation in one place.
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.
looks like txSize is still calculated twice
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.
Reverted.
fedd472
to
170b87a
Compare
23c49dd
to
b44f884
Compare
r+ b44f884 |
…from txn and overlay systems
Description
Resolves #4023
Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)