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 a util function to construct the create2 salt #814

Merged
merged 5 commits into from
Feb 6, 2024

Conversation

andreivladbrg
Copy link
Member

@andreivladbrg andreivladbrg commented Feb 5, 2024

Addresses #810

I decided to log the string salt to visually see what is being passed (also testing it, in case something is not working).

@smol-ninja, can you check, please, if I missed anything in the multi-chain script?

smol-ninja
smol-ninja previously approved these changes Feb 5, 2024
Copy link
Member

@smol-ninja smol-ninja left a comment

Choose a reason for hiding this comment

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

LGTM! I haven't tested the scripts locally though.

@PaulRBerg
Copy link
Member

Looks like the fork test fails are due to QuickNode?

SCR-20240205-qebb

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.

I like this idea a lot - but we should absolutely test this new function in test/utils.

script/Base.s.sol Outdated Show resolved Hide resolved
script/Base.s.sol Show resolved Hide resolved
script/Base.s.sol Outdated Show resolved Hide resolved
script/Base.s.sol Outdated Show resolved Hide resolved
@andreivladbrg
Copy link
Member Author

I don't think is related to QuickNode, since in periphery worked: sablier-labs/v2-periphery#280

@smol-ninja
Copy link
Member

That looks like rate limiting imposed by Quicknode (2500 credits / sec).

@PaulRBerg
Copy link
Member

PaulRBerg commented Feb 6, 2024

Bummer. Let's try another provider - I will switch to ChainNodes.

Edit: done.

chore: remove underscore from function name
@andreivladbrg
Copy link
Member Author

andreivladbrg commented Feb 6, 2024

but we should absolutely test this new function in test/utils.

note: it would require to include --ffi flag in the test commands (locally) otherwise the tests would fail. e.g. forge test --no-match-test "testFork" --ffi

do you have any solution on how to fix this in the CI? how to include a flag in the command here:

https://github.com/sablier-labs/v2-core/blob/2b65c464a40fe0924ef1c3ce0aa24a0d90b3abf8/.github/workflows/ci.yml#L39-L45

@PaulRBerg
Copy link
Member

do you have any solution on how to fix this in the CI?

Yes. We can add ffi = true in the Foundry config file.

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.

See below

foundry.toml Show resolved Hide resolved
@andreivladbrg
Copy link
Member Author

@PaulRBerg does it look good now? can you approve the PR?

will merge it once the CI is over

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.

LGTM now

@andreivladbrg andreivladbrg merged commit 1f09215 into main Feb 6, 2024
9 checks passed
@andreivladbrg andreivladbrg deleted the feat/util-for-salt branch February 8, 2024 15:15
@PaulRBerg
Copy link
Member

This PR continues to be amazingly useful - @andreivladbrg you should tweet about it.

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