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

Abi errors RFC #43

Merged
merged 8 commits into from
Nov 29, 2024
Merged

Abi errors RFC #43

merged 8 commits into from
Nov 29, 2024

Conversation

IGI-111
Copy link
Contributor

@IGI-111 IGI-111 commented Nov 25, 2024

@IGI-111 IGI-111 requested review from a team November 25, 2024 07:32
@IGI-111 IGI-111 self-assigned this Nov 25, 2024
@IGI-111 IGI-111 requested a review from a team November 25, 2024 07:35
Copy link

@hal3e hal3e left a comment

Choose a reason for hiding this comment

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

Great rfc. This should make the devx so much better.

  • I would like to work on this on the rust sdk side.

@tritao
Copy link
Contributor

tritao commented Nov 25, 2024

It looks great and a really welcome improvement to error handling.

The only concern I have is how the syntax at the moment is directed specifically towards ABI error handling, and this functionality seems like it could be pretty useful just as a general error-handling functionality as well, without necessarily being ABI-specific. This ABI terminology feels like it would be a bit out of place if in the future we have some other targets outside the Fuel VM in which the ABI meaning is not as clear-defined.

What if we made the terminology around this a bit more generic, like Rust, specifically an Error trait analogous to Rust std::error::Error, equivalent to your AbiError.

The main direct issue I can see with this is that if we also dropped abi_ prefix from abi_error attribute, then we would have a clash between error for the main error attribute and error attribute for each individual enum cases.

Rust side steps that issue by using derive instead, which could work for this as well, and would prevent a special attribute like abi_error, we would just use derive(Error). We would need to pick up on FuelLabs/sway#2396 and get the basic derive machinery in place.

@IGI-111
Copy link
Contributor Author

IGI-111 commented Nov 25, 2024

I deliberately didn't want to use "derive", but I'm not opposed to renaming the trait to Error and finding a better name for the type annotation.

Maybe #[error_type]?

rfcs/0014-abi-errors.md Outdated Show resolved Hide resolved
rfcs/0014-abi-errors.md Outdated Show resolved Hide resolved
rfcs/0014-abi-errors.md Outdated Show resolved Hide resolved
rfcs/0014-abi-errors.md Outdated Show resolved Hide resolved
rfcs/0014-abi-errors.md Show resolved Hide resolved
rfcs/0014-abi-errors.md Outdated Show resolved Hide resolved
@alfiedotwtf
Copy link

Looks good and makes sense. Fix the typo and I'm happy to approve :)

Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

Looks great to me! This will be a big step forward in error handling.

Other than two spelling typos shown in:

https://github.com/FuelLabs/sway-rfcs/pull/43/files#r1857256227
https://github.com/FuelLabs/sway-rfcs/pull/43/files#r1857482020

It seems good to go to me!

@JoshuaBatty
Copy link
Member

Really great work on this.

Copy link

@esdrubal esdrubal left a comment

Choose a reason for hiding this comment

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

The specification seems to solve all the problems with current reverts, adding msg outputs and line number information to the ABI json. Nice work!

Copy link
Member

@ironcev ironcev left a comment

Choose a reason for hiding this comment

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

Great proposal! Straightforward to implement and brings a huge benefit. This is a kind of low hanging fruit I wish we identify more.

rfcs/0014-abi-errors.md Outdated Show resolved Hide resolved
rfcs/0014-abi-errors.md Outdated Show resolved Hide resolved
rfcs/0014-abi-errors.md Show resolved Hide resolved
rfcs/0014-abi-errors.md Show resolved Hide resolved
rfcs/0014-abi-errors.md Show resolved Hide resolved
rfcs/0014-abi-errors.md Outdated Show resolved Hide resolved
rfcs/0014-abi-errors.md Outdated Show resolved Hide resolved
rfcs/0014-abi-errors.md Outdated Show resolved Hide resolved
rfcs/0014-abi-errors.md Outdated Show resolved Hide resolved
rfcs/0014-abi-errors.md Show resolved Hide resolved
Copy link

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Great work on this @IGI-111

Not strong opinion on this suggestion so feel free to resolve if you don't think it makes the intention clearer.

I'd give my explicit approval but compiler level changes are beyond my scope 😅

I approve in theory though 😄

Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

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

Great work, this is going to be a huge improvement! Just left a couple of suggestions/questions.

rfcs/0014-abi-errors.md Show resolved Hide resolved
rfcs/0014-abi-errors.md Show resolved Hide resolved
@IGI-111 IGI-111 mentioned this pull request Nov 29, 2024
4 tasks
@IGI-111 IGI-111 merged commit 443ef9e into master Nov 29, 2024
2 checks passed
@IGI-111 IGI-111 deleted the IGI-111/abi-errors branch November 29, 2024 08:16
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.