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

use ERC20_MAIN macro #99

Merged
merged 2 commits into from
Feb 20, 2023
Merged

use ERC20_MAIN macro #99

merged 2 commits into from
Feb 20, 2023

Conversation

AmadiMichael
Copy link
Contributor

Changing no-match jumpi to no-match jump in ERC20.huff function dispatcher leaves the shifted function sig on the stack for contracts inheriting it which can use it after the no-match JUMPDEST. This way, importing (wrapping) contracts only have to add:

0x00 calldataload 0xE0 shr
ERC20_MAIN()

to the start of their MAIN() macro then add their specific function below it rather than copying all of ERC20.huff dispatch macro.

@devtooligan
Copy link
Collaborator

Hey frenz, thanks for working on this one. tbh I'm not 100% sure whats going on. Looks like were removing some macro calls because they already exist in the parent or smthg? i dont get this comment:

Changing no-match jumpi to no-match jump in ERC20.huff function dispatcher

I dont see that change to jump here?

Also it looks like @MathisGD is on this so I'll let you guys finish this up, lemme know if I can help.

@AmadiMichael
Copy link
Contributor Author

Hey frenz, thanks for working on this one. tbh I'm not 100% sure whats going on. Looks like were removing some macro calls because they already exist in the parent or smthg? i dont get this comment:

Changing no-match jumpi to no-match jump in ERC20.huff function dispatcher

I dont see that change to jump here?

Also it looks like @MathisGD is on this so I'll let you guys finish this up, lemme know if I can help.

Yeah, thanks!
Referencing the no-match jumpi part, I originally looked at the main branch which used no-match jumpi which would have popped off the func sig and needing extra operations to get it back on the stack to be used by wrapping contracts.

I was thinking that was why the function dispatching part of the erc20 main was copied into the Wrapping contract.

But under the hood like @MathisGD said, it would look the same way and is more of a preference stuff.

The original pr I wanted to make was Changing the jumpi to jump to enable all this

@devtooligan
Copy link
Collaborator

i see. so can this pr be closed now? or whats the path forward?

@MathisGD
Copy link
Collaborator

I still think that it is better not to have the code duplication.

@devtooligan
Copy link
Collaborator

Agree - @MathisGD So all good to merge this?

@MathisGD
Copy link
Collaborator

MathisGD commented Feb 20, 2023

I would have harmonized the two contracts (#99 (comment)) but that's not a big deal

@AmadiMichael
Copy link
Contributor Author

I would have harmonized the two contracts (#99 (comment)) but that's not a big deal

Is the current commit okay?

Copy link
Member

@Maddiaa0 Maddiaa0 left a comment

Choose a reason for hiding this comment

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

ye haw

@Maddiaa0 Maddiaa0 merged commit 978c130 into huff-language:dev Feb 20, 2023
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.

4 participants