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

refactor: add error handling for Bytecode type #379

Merged
merged 12 commits into from
Oct 16, 2024

Conversation

TropicalDog17
Copy link
Contributor

Fixes #378

@TropicalDog17
Copy link
Contributor Author

\cc @hai-rise

@TropicalDog17
Copy link
Contributor Author

Seem the tests is falling, I will try to fix asap @hai-rise

@hai-rise
Copy link
Contributor

@TropicalDog17 Yes, conversions in the main library must also handle the new Result. Thank you for working on this 🙏!

@TropicalDog17
Copy link
Contributor Author

Hi @hai-rise, seems CI is passing now, can you take a look :)

Copy link
Contributor

@hai-rise hai-rise left a comment

Choose a reason for hiding this comment

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

I've left some comments. Some parts can be quite confusing. We should definitely improve error handling going forward; or really inline an EVM implementation to avoid these REVM conversions altogether.

src/storage.rs Outdated Show resolved Hide resolved
src/storage.rs Show resolved Hide resolved
src/storage.rs Show resolved Hide resolved
src/storage.rs Outdated Show resolved Hide resolved
src/storage.rs Show resolved Hide resolved
src/storage.rs Outdated Show resolved Hide resolved
src/vm.rs Outdated Show resolved Hide resolved
src/vm.rs Show resolved Hide resolved
src/storage.rs Outdated Show resolved Hide resolved
src/storage.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@hai-rise hai-rise left a comment

Choose a reason for hiding this comment

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

More nits 🙏.

Cargo.toml Outdated Show resolved Hide resolved
src/storage.rs Outdated Show resolved Hide resolved
src/storage.rs Outdated Show resolved Hide resolved
src/storage.rs Outdated Show resolved Hide resolved
src/storage.rs Outdated Show resolved Hide resolved
src/vm.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@hai-rise hai-rise left a comment

Choose a reason for hiding this comment

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

Almost there!

Cargo.toml Show resolved Hide resolved
src/storage.rs Outdated Show resolved Hide resolved
src/storage.rs Outdated Show resolved Hide resolved
src/storage.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@hai-rise hai-rise left a comment

Choose a reason for hiding this comment

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

Thank you for your patience! Hopefully, #382 will also remove most of these complications in the future.

@hai-rise hai-rise merged commit 8bedf9f into risechain:main Oct 16, 2024
1 check passed
@TropicalDog17
Copy link
Contributor Author

Thank you for your patience! Hopefully, #382 will also remove most of these complications in the future.

Yeah I'm also very appreciated your detailed review @hai-rise

@TropicalDog17 TropicalDog17 deleted the refactor/bytecode-tryfrom branch October 17, 2024 08:41
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.

Better error handling for EvmCode to Bytecode conversion
2 participants