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

250 improve error handling in chain extension #349

Merged
merged 5 commits into from
Nov 23, 2023

Conversation

gianfra-t
Copy link
Contributor

@gianfra-t gianfra-t commented Nov 16, 2023

Closes #250 .

This PR proposes to use the existing ChainExtensionError as the enum discussed in the issue, which is now renamed ChainExtensionOutcome which also holds the Success variant.

For this, each value is mapped to a corresponding integer, and later we use the corresponding ChainExtensionOutcome as u32 when returning.

A conversion from u32 to ChainExtensionOutcome or it's inner variants is also defined

@gianfra-t gianfra-t linked an issue Nov 16, 2023 that may be closed by this pull request
@gianfra-t gianfra-t requested a review from a team November 17, 2023 13:16
@gianfra-t gianfra-t marked this pull request as ready for review November 17, 2023 13:16
Copy link
Member

@ebma ebma left a comment

Choose a reason for hiding this comment

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

Looks good overall 👍
A few remarks that came to my mind.

  1. It's probably a good idea to return a value that indicates 'success' for chain extension functions that are changing state. I can see that Astar is returning a 'Success'-error for each mutating function.
  2. We might also want to consider implementing a conversion from number -> Error type, like eg. here. This way, the error enum could also be re-used in ink! smart contracts that want to interact with our chain extension. (This is not super useful until we extracted the chain extension into an extra crate here, but still nice to have).

@gianfra-t
Copy link
Contributor Author

I understand the conversion back from number to chain extension error, and I think also returning a success is more readable than 0. Then I would propose to rename the enum to something that is not ChainExtensionError, so that it does not become confusing (naming it Outcome, let's say)

@ebma
Copy link
Member

ebma commented Nov 22, 2023

Then I would propose to rename the enum to something that is not ChainExtensionError, so that it does not become confusing (naming it Outcome, let's say)

Sure. I would be fine with ChainExtensionOutcome. We could also do ChainExtensionRetVal or ChainExtensionReturnValue. Not sure what's best, WDYT @gianfra-t and @b-yap?

@gianfra-t
Copy link
Contributor Author

I've added the conversion from u32 to ChainExtensionOutcome and adjusted the code to the refactor made in a recently merged PR.

Copy link
Contributor

@b-yap b-yap left a comment

Choose a reason for hiding this comment

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

No complaints from me.

Except for that 1 CI error

@ebma
Copy link
Member

ebma commented Nov 23, 2023

I think the CI basically passed because only saving the cache failed, right? Maybe we can just merge it like this

@gianfra-t
Copy link
Contributor Author

Does that step in the CI just cleans after the cargo build? In that case yes we should already merge it. Otherwise we can re-check again.

@ebma
Copy link
Member

ebma commented Nov 23, 2023

I think it's fine to ignore. I'll merge it now.

@ebma ebma merged commit 8050ecf into main Nov 23, 2023
1 of 2 checks passed
@ebma ebma deleted the 250-improve-error-handling-in-chain-extension branch November 23, 2023 14:04
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.

Improve error handling in chain extension
3 participants