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

feat: add byte array type and string conversions #522

Closed
wants to merge 7 commits into from

Conversation

glihm
Copy link
Contributor

@glihm glihm commented Dec 10, 2023

This PR aims at bringing support for ByteArray type.

@glihm
Copy link
Contributor Author

glihm commented Dec 10, 2023

I think I've JSON rpc related errors, if you have the opportunity to manually trigger the workflow it could be great @xJonathanLEI. Thanks a lot!

@glihm
Copy link
Contributor Author

glihm commented Dec 18, 2023

I'm thinking that we should introduce a Bytes31 type to ensure that the data is actually with the correct format. WDYT @xJonathanLEI?

EDIT: useless question sorry, will implement it in the PR. 👍

@glihm glihm marked this pull request as draft January 8, 2024 15:36
@glihm glihm force-pushed the feat/type-byte-array branch from 955820f to 0ddf5f4 Compare June 21, 2024 05:06
@glihm glihm marked this pull request as ready for review June 21, 2024 05:08
@glihm
Copy link
Contributor Author

glihm commented Jun 21, 2024

@xJonathanLEI here's the updated version of the PR. Few months of blank here, sorry for that.

I've added the Bytes31 type to be consistent with Cairo.

Happy to address any comment!

@glihm
Copy link
Contributor Author

glihm commented Dec 2, 2024

@xJonathanLEI rebased on master, happy to dive in any update as necessary.

@xJonathanLEI
Copy link
Owner

Hey @glihm thanks for the PR. I see you're modelling the types here directly after their Cairo implementations, with the internals exposed.

It kinda makes me wonder though: do we really expect users to care about what a particular array's data, pending_word, and pending_word_len really are?

I'm not exactly convinced that it's the case. The exact composition should only matter when crossing the Rust <-> Cairo boundary (i.e. encoding/decoding), which should be mostly opaque to users. Users most likely just want to deal with bytes.

So IMO a better design would be a fully encapsulated ByteArray type that's internally backed by Vec<u8>. We could provide getters for viewing the components like data, pending_word, and pending_word_len, but I also don't think that's actually needed.

Then we should have this type implement Encode and Decode to make crossing the Cairo boundary seamless.

@xJonathanLEI
Copy link
Owner

Superseded by #682.

@elton-cs
Copy link

No way! I was just looking for a ByteArray implementation and turns out it was implemented 5 days ago! Man, we really are on the bleeding edge haha 😅. Thanks for the new feature guys!

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.

3 participants