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

Add support for CREATE2 clones and address prediction #19

Merged
merged 4 commits into from
Jan 17, 2024

Conversation

Arachnid
Copy link
Contributor

No description provided.

@wighawag
Copy link
Owner

Thanks @Arachnid for the PR, I posted a comment on the CREATE3 PR: #17 where I mentioned that it would be good to also think about CREATE2 in regard to naming convention

right now the naming convention might be pretty confusing if we accept both PR.

clone2 might work but predictAddress could apply to both

I did not think that through but cloneWithCREATE2 and predictAddressOfCloneWithCREATE2 would at least have more clear meaning.

What do you think ? @ZeframLou ?

@Arachnid
Copy link
Contributor Author

I think clone2 is to clone as create2 is to create, so that naming makes sense, while also remaining fairly compact.

predictAddressOfCloneWithCREATE2 seems way too long. Could we do something like addressOfClone2?

@wighawag
Copy link
Owner

Yes, clone2 and addressOfClone2 works for me

@ZeframLou
Copy link
Contributor

Yeah this seems good, will update my PR

@Arachnid
Copy link
Contributor Author

I've done the rename, and also refactored the library to return creation bytecode, which is necessary in order for addressOfClone2 to be a view function.

Unfortunately this does have some impact on gas usage; testGas_clone goes from 64,579 gas to 64,738 (+159 gas).

@wighawag
Copy link
Owner

Thanks for that.

I am personally ok with extra gas but another solution would be to duplicate the code between the view function and the write function at the cost of later maintainability.

@Arachnid
Copy link
Contributor Author

I did consider that, but it seems like a lot of bloat and maintenance hassle.

@wighawag wighawag merged commit 0326de7 into wighawag:master Jan 17, 2024
1 check passed
This was referenced Jan 17, 2024
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