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

Make from_hex() safer in rust bindings #307

Merged
merged 2 commits into from
May 28, 2023
Merged

Make from_hex() safer in rust bindings #307

merged 2 commits into from
May 28, 2023

Conversation

jtraglia
Copy link
Member

This PR makes it so from_hex cannot panic. Previously, it would panic if the string were fewer than 2 bytes or the hex was invalid. This makes the "0x" optional, which is more user friendly IMO; like the Go bindings.

@asn-d6
Copy link
Contributor

asn-d6 commented May 26, 2023

This LGTM but let's also get @divagant-martian or @pawanjay176 to check it out.

Copy link
Contributor

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

LGTM. Just a small suggestion, I'm okay either way.

bindings/rust/src/bindings/mod.rs Outdated Show resolved Hide resolved
@asn-d6
Copy link
Contributor

asn-d6 commented May 27, 2023

121c6bb LGTM. I will wait for @divagant-martian and @pawanjay176 to take a look as well before merging.

(Accidentally misclicked and closed the PR. Reopened it)

@asn-d6 asn-d6 closed this May 27, 2023
@asn-d6 asn-d6 reopened this May 27, 2023
Copy link
Contributor

@divagant-martian divagant-martian left a comment

Choose a reason for hiding this comment

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

LGTM! nice and clean

@asn-d6
Copy link
Contributor

asn-d6 commented May 28, 2023

Thanks all!

@asn-d6 asn-d6 merged commit 6c50a43 into ethereum:main May 28, 2023
59 checks passed
@jtraglia jtraglia deleted the safe-fromhex branch May 28, 2023 21:29
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.

None yet

4 participants