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: include shape in merkle contracts #18

Merged
merged 7 commits into from
Nov 27, 2024
Merged

Conversation

smol-ninja
Copy link
Member

@smol-ninja smol-ninja commented Nov 26, 2024

Closes #1.

Changelog:

  • Generates salt by encoding baseParams instead encoding parameters separately. This helps to prevent "stack too deep error" caused after adding shape in keccak256(abi.encodePacked(..)) of salt.
  • Refactors asset to token as a result of Refactor asset to token v2-core#1097.
  • Adds a variable shape of type string. Instead of reverting on longer than 32 chars length, it truncates the name to 32 chars.
  • Reorders some test assertions alphabetically.
  • Renames NAME to CAMPAIGN_NAME in Defaults contract.

@andreivladbrg the idea of storing shape as bytes32 did not work because when the string of length less than 32 characters is stored as bytes32, it adds 0 to make it fit into 32 bytes slot which changes the length of the string upon conversion back to string. On the other hand, storing it as a string datatype preserves the string length as well in the storage. Therefore I decided to store it as a string and truncates its length if the length exceeds 32 characters.

@smol-ninja smol-ninja changed the title feat: include shape name in merkle contracts feat: include shape in merkle contracts Nov 26, 2024
Copy link
Member

@PaulRBerg PaulRBerg left a comment

Choose a reason for hiding this comment

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

feedback below

src/abstracts/SablierMerkleBase.sol Show resolved Hide resolved
src/abstracts/SablierMerkleBase.sol Outdated Show resolved Hide resolved
src/types/DataTypes.sol Outdated Show resolved Hide resolved
Co-authored-by: Paul Razvan Berg <prberg@proton.me>
Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

generally LGTM 👍

src/SablierMerkleFactory.sol Show resolved Hide resolved
src/abstracts/SablierMerkleBase.sol Show resolved Hide resolved
src/interfaces/ISablierMerkleBase.sol Outdated Show resolved Hide resolved
src/abstracts/SablierMerkleBase.sol Outdated Show resolved Hide resolved
refactor: rename var to campaignName
refactor: store the campaign name as string
test: update tests accordingly
@andreivladbrg
Copy link
Member

pushed a commit to address the feedback, lmk if it looks good @smol-ninja

@smol-ninja
Copy link
Member Author

Yes. Thanks for the commit. Looks good to me. Can you please approve the PR if there are no more suggestions?

Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

r_downey_thumbs_up

@andreivladbrg andreivladbrg merged commit 012c8b2 into main Nov 27, 2024
7 checks passed
@andreivladbrg andreivladbrg deleted the refactor/shape-name branch November 27, 2024 16:16
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.

Account for string in Create parameters
3 participants