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

Decorate error for nonce missing from the footprint. #1112

Merged
merged 3 commits into from
Oct 13, 2023

Conversation

dmkozh
Copy link
Contributor

@dmkozh dmkozh commented Oct 13, 2023

What

Decorate error for nonce missing from the footprint.

Why

Improving error diagnostics

Known limitations

N/A

@graydon
Copy link
Contributor

graydon commented Oct 13, 2023

I think this is plausible but it gets at something I've been thinking of a few times during review: we have a few subsystems (storage, metered maps and such) that take a Budget rather than Host (in order to enable working with them before/after Host lifecycle is complete, while still metering things) but by doing that we lose the ability to record detailed causes of failure in those context the way we do with Host::error. I was thinking it might make sense to turn some of those methods into impl AsBudget, and extend the AsBudget trait to include some of the error-creation-and-reporting functions in Host so that when we do call something from a Host context, we can record the extra context. The default methods of AsBudget (or definitions in impl AsBudget for Budget) could just construct and return the error without decoration. WDYT?

@dmkozh
Copy link
Contributor Author

dmkozh commented Oct 13, 2023

WDYT?

As per offline discussion, it would be generally nicer than returning event-less ScErrors, but decoration is still required in some cases in order to provide more rich and contextual message and meaningful context (e.g. in this particular case there is no way to convert the LedgerKey missing from the footprint to Val, thus we explicitly state that nonce entry is missing in the error message).

This seems to be the only object error that is easy to encounter without tampering with `Val`s.
@graydon graydon added this pull request to the merge queue Oct 13, 2023
Merged via the queue into stellar:main with commit 088d49d Oct 13, 2023
9 checks passed
@leighmcculloch
Copy link
Member

I think it's concerning that we can have errors that don't feed into diagnostic events. I understand we might miss decorating errors with additional information in some places, but is there a way to restructure the errors such that regardless of whether they get decorated or not, the most basic information about them still end up in the diagnostic events?

@graydon
Copy link
Contributor

graydon commented Oct 14, 2023

@leighmcculloch Errors are generated in contexts where there is no diagnostic event buffer to record them into, so not "in general", but we can do a little better in a few contexts, probably. We'll keep iterating on this, the set of diagnostic events can improve over time without breaking anything.

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.

3 participants