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.

Sign up for GitHub

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

Jump to bottom

feat: bytes to base64 encoding in VM #6785

Merged
merged 6 commits into from
Jan 14, 2024
Merged

feat: bytes to base64 encoding in VM #6785

merged 6 commits into from
Jan 14, 2024

Conversation

0xAndoroid
Copy link
Contributor

@0xAndoroid 0xAndoroid commented Jan 13, 2024 •
edited by DaniPopes
Loading

Motivation

Closes #6744

Solution

  • Add function signatures
  • Add cheatcode implementation
  • Add tests

DaniPopes
DaniPopes reviewed Jan 13, 2024
View reviewed changes
crates/cheatcodes/spec/src/vm.rs Outdated

/// Encodes a `string` value to base64 string
#[cheatcode(group = Base64)]
function toBase64(string calldata data) external pure returns (string memory);
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we don't need functions for both bytes and string because bytes alone suffices.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is bytes alone preferable? My understanding is the primary use case here will be for encoding/decoding tokenURIs which are strings, so if anything it seems string only would be preferable to avoid unnecessary casting? However looking at existing Base64 libs (OZ and solady) they also use bytes input, so maybe I'm missing something

Copy link
Member

@DaniPopes DaniPopes Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thought strings coerce to bytes but that's only for literals. It's not that big of a deal if we can support both.

#6830

DaniPopes
DaniPopes requested changes Jan 14, 2024
View reviewed changes
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

I believe this can be just abi_encoded.

crates/cheatcodes/src/base64.rs Outdated Show resolved Hide resolved
crates/cheatcodes/src/base64.rs Outdated Show resolved Hide resolved
DaniPopes
DaniPopes approved these changes Jan 14, 2024
View reviewed changes
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM! FYI I've edited the description to use GitHub keywords when referencing issues.

@DaniPopes DaniPopes merged commit bb35926 into foundry-rs:master Jan 14, 2024
20 checks passed
@0xAndoroid
Copy link
Contributor Author

Thanks for your help!

@0xAndoroid 0xAndoroid deleted the base64_cheatcode branch January 14, 2024 03:26
@mds1 mds1 mentioned this pull request Jan 16, 2024
Closed
@DaniPopes DaniPopes mentioned this pull request Jan 17, 2024
Merged
This was referenced Apr 26, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DaniPopes DaniPopes DaniPopes approved these changes

@mds1 mds1 mds1 left review comments

@Evalir Evalir Awaiting requested review from Evalir

@mattsse mattsse Awaiting requested review from mattsse mattsse is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add bytes to Base64 encoding in VM
3 participants
@0xAndoroid @mds1 @DaniPopes