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

Rework error naming #671

Closed
Fumuran opened this issue May 7, 2024 · 5 comments
Closed

Rework error naming #671

Fumuran opened this issue May 7, 2024 · 5 comments
Assignees
Milestone

Comments

@Fumuran
Copy link
Contributor

Fumuran commented May 7, 2024

What should be done?

Change the names of the errors so that the essence of the error is clear only by name.
Change grouping of the errors so that their finding will be easier and faster.

How should it be done?

The names of the constants representing the error codes should be changed both in the .rs and .masm files.
Error codes constants in the miden-tx/src/error.rs file should be divided into groups.

When is this task done?

The task is done when error constants have the proper easy to understand names listed by groups.

Additional context

Original comment: #634 (review)

@bobbinth bobbinth added this to the v0.4 milestone May 14, 2024
@Fumuran Fumuran linked a pull request May 28, 2024 that will close this issue
@bobbinth bobbinth modified the milestones: v0.4, v0.5 Jul 4, 2024
@Dominik1999 Dominik1999 moved this to In Review in User's testnet Aug 12, 2024
@bobbinth bobbinth modified the milestones: v0.5, v0.6 Aug 27, 2024
@PhilippGackstatter
Copy link
Contributor

PhilippGackstatter commented Oct 2, 2024

I mentioned on Discord that it should be possible to extract the error constants and their associated error message automatically in a build.rs file from the kernel masm files.

For a given masm file, we extract the comment before the constants definition as the error message, e.g.

# Asset burn can not exceed the existing supply
const.ERR_FAUCET_BURN_OVER_ISSUANCE=0x00020023

would turn into

pub const ERR_FAUCET_BURN_OVER_ISSUANCE: u32 = 0x00020023;

and

pub const KERNEL_ERRORS: [(u32, &str); _] = [
    (ERR_FAUCET_BURN_OVER_ISSUANCE, "Asset burn can not exceed the existing supply"),
];

so the same as now, just automatically.

The reworked error names from #712 could still be used, but using the error message from the comment allows for a better error, since one can elaborate more in an error message than in a constant's name, one can give better context and generally make the error read more like a natural sentence.

With error messages we can give context from where the error originated like "transaction prologue: new account vault must be empty" whereas in the constant ERR_PROLOGUE_NEW_ACCOUNT_VAULT_MUST_BE_EMPTY it's included, but much more implicit for users that are less familiar with it.

Alternative

The error names are already much better and quite self-sufficient with #712, so if the above is somehow not possible or desirable then I would propose to still use a build.rs script and extract only the constants, and rewrite the constant names to nicer strings, e.g. ERR_PROLOGUE_NEW_ACCOUNT_VAULT_MUST_BE_EMPTY -> "prologue: new account vault must be empty". Here the ERR_ could be ignored, and the second part of the constant could be used as the context and postfixed with a :. But it likely won't always be possible to express the context in a single word or some developers might not be aware of this implicit constraint and then it'll cause some weird error messages. Overall, I would be in favor of the "comment extraction" approach due to its greater flexibility.

Also I'm happy to work on this 🙂

@bobbinth
Copy link
Contributor

bobbinth commented Oct 2, 2024

For a given masm file, we extract the comment before the constants definition as the error message, e.g.

# Asset burn can not exceed the existing supply
const.ERR_FAUCET_BURN_OVER_ISSUANCE=0x00020023

would turn into

pub const ERR_FAUCET_BURN_OVER_ISSUANCE: u32 = 0x00020023;

and

pub const KERNEL_ERRORS: [(u32, &str); _] = [
    (ERR_FAUCET_BURN_OVER_ISSUANCE, "Asset burn can not exceed the existing supply"),
];

I like this! Let's do this and then this will supersede #712. I'll assign this issue to you.

@Fumuran
Copy link
Contributor Author

Fumuran commented Oct 2, 2024

@PhilippGackstatter Awesome idea! This approach will make the error maintenance a lot more easy.
But some renaming for the error constant is still needed: while updating the errors I noticed that some of them have mistaken or inaccurate names and comments, so ideally we should update them. Probably you can use my renamed errors to spot ones which should be updated.

@PhilippGackstatter
Copy link
Contributor

But some renaming for the error constant is still needed: while updating the errors I noticed that some of them have mistaken or inaccurate names and comments, so ideally we should update them. Probably you can use my renamed errors to spot ones which should be updated.

Thanks for the hint! I'll build on top of your work then 👍

@bobbinth
Copy link
Contributor

Superseded by the work done in #906.

@github-project-automation github-project-automation bot moved this from In Review to Done in User's testnet Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants