Skip to content
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

Include ledger sequence in preflight response #3560

Merged
merged 1 commit into from
Oct 3, 2022

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented Oct 3, 2022

Description

This commit adds a ledger sequence to the preflight response which is useful because execution of a smart contract will depend on the state of the ledger when the operation is applied.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@tamirms tamirms changed the title Include leddger sequence in preflight response Include ledger sequence in preflight response Oct 3, 2022
Copy link

@paulbellamy paulbellamy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, but idk

@@ -220,9 +220,11 @@ InvokeHostFunctionOpFrame::preflight(Application& app,
{
auto cb = std::make_unique<PreflightCallbacks>(app);
Json::Value root;
LedgerTxn ltx(app.getLedgerTxnRoot());
root["ledger"] = ltx.loadHeader().current().ledgerSeq;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for moving ltx out of the try/catch block? The constructor call throw and this would crash the node instead of returning an error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed that if the invoke function operation failed (e.g. the smart contract reverted) then that would throw an exception and we would enter the catch clause. In that casem it would still be nice to know the ledger sequence of the invoke function attempt.

I assumed that moving ltx out of the try/catch block should be safe because we already do that in the get ledger entry endpoint:

LedgerTxn ltx(mApp.getLedgerTxnRoot());
root["ledger"] = ltx.loadHeader().current().ledgerSeq;

Note how the same code is executed without any try / catch block in the handler.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually commandhandler has a try/catch anyways at the top level (just returns a 500) so we're good

@MonsieurNicolas
Copy link
Contributor

r+ e7f06ea

@latobarita latobarita merged commit b4085c1 into stellar:master Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants