-
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
Don't generate diagnostic errors at all when diagnostics is disabled. #4094
Conversation
Also added diagnostic messages to a few errors that didn't have them.
LCM test doesn't actually have diagnostics enabled, we've returned one event due to an issue fixed in the previous commit.
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.
LGTM, a few nit picks.
pushSimpleDiagnosticError( | ||
cfg, SCE_BUDGET, SCEC_EXCEEDED_LIMIT, | ||
"refundable resource fee was not sufficient to cover the events " | ||
"fee after paying for ledger storage rent: {} > {}", |
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 think we've moved away from "rent" terminology in the docs, instead of "ledger storage rent" what about "storage TTL fee"?
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.
Have we though? I thought we're still naming the payment 'rent payment' (and as result it extends TTL). The fees doc still talks about 'paying rent', which makes sense to me.
@@ -154,12 +154,18 @@ ExtendFootprintTTLOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx, | |||
|
|||
bool | |||
ExtendFootprintTTLOpFrame::doCheckValid(SorobanNetworkConfig const& config, |
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.
Nit: change config
to networkConfig
or sorobanConfig
for clarity, both here and in the other ops that now take both a network config and app config as parameters.
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.
Done.
@@ -168,6 +174,9 @@ ExtendFootprintTTLOpFrame::doCheckValid(SorobanNetworkConfig const& config, | |||
if (!isSorobanEntry(lk)) | |||
{ | |||
innerResult().code(EXTEND_FOOTPRINT_TTL_MALFORMED); | |||
mParentTx.pushSimpleDiagnosticError( | |||
appConfig, SCE_STORAGE, SCEC_INVALID_INPUT, | |||
"only entries with TTL can have it extended", {}); |
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 don't think this is a clear error the the user, if I saw this I'd google how to create a TTL entry. What about "only soroban entries can extend TTL" or "only contract data and code entries can extend TTL".
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.
Updated.
@@ -360,7 +360,7 @@ InvokeHostFunctionOpFrame::doApply(Application& app, AbstractLedgerTxn& ltx, | |||
ttlEntryCxxBufs.reserve(footprintLength); | |||
|
|||
auto addReads = [&ledgerEntryCxxBufs, &ttlEntryCxxBufs, <x, &metrics, | |||
&resources, &sorobanConfig, | |||
&resources, &sorobanConfig, &cfg, |
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.
Nit: cfg
-> appCfg
or appConfig
for clarity and consistency.
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.
Done.
src/transactions/TransactionUtils.h
Outdated
@@ -4,6 +4,7 @@ | |||
// under the Apache License, Version 2.0. See the COPYING file at the root | |||
// of this distribution or at http://www.apache.org/licenses/LICENSE-2.0 | |||
|
|||
#include "main/Config.h" |
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.
Nit: forward declare config instead of include.
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.
Done.
77fc840
to
ccf5c1c
Compare
r+ ccf5c1c |
Description
Don't generate diagnostic errors at all when diagnostics is disabled.
Also added diagnostic messages to a few errors that didn't have them.
Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)