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

[Feature] FixedBytes should map to FixedBytes #259

Closed
rachel-bousfield opened this issue Sep 1, 2023 · 4 comments · Fixed by #276
Closed

[Feature] FixedBytes should map to FixedBytes #259

rachel-bousfield opened this issue Sep 1, 2023 · 4 comments · Fixed by #276
Assignees
Labels
enhancement New feature or request

Comments

@rachel-bousfield
Copy link
Contributor

rachel-bousfield commented Sep 1, 2023

Component

primitives, sol-type

Describe the feature you would like

Right now the SolType impl for sol_data::FixedBytes has the RustType [u8; N].

We've worked around this in the Stylus SDK by introducing a new type, abi::FixedBytesSolType, and foresee potential special casing around this quirk.

We propose that, as is already the case with integers, sol_data::FixedBytes have the RustType alloy_primitives::FixedBytes.

We're happy to help implement this change, which should be straightforward :)

@DaniPopes
Copy link
Member

DaniPopes commented Sep 1, 2023

SGTM, I'll assign you to this issue then! We'll be releasing 0.4.0 with these changes.

@prestwich
Copy link
Member

I'm on board with this change

The main impact on users will be the change to the output type of decoding for sol types containing a bytesN, which seems fine. Can you make sure this change is propagated to the DynSolValue::FixedBytes as well, by having it wrap a FixedBytes<32> instead of a [u8;32]

@DaniPopes
Copy link
Member

Hey @rachel-bousfield, do you still plan on working on this? Otherwise I can take over this issue

@rachel-bousfield
Copy link
Contributor Author

Hey @DaniPopes feel free to take it -- we won't have time for the next couple of weeks but probably will after that. Happy either way and thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants