search

huff-language / huffmate

A library of modern, hyper-optimized, and extensible Huff contracts with extensive testing and documentation built by Huff maintainers.
https://github.com/pentagonxyz/huffmate
MIT License
430 stars 55 forks source link

use ERC20_MAIN macro #99

Closed AmadiMichael closed 1 year ago

AmadiMichael commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

MathisGD commented 1 year ago

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

devtooligan commented 1 year ago

Agree - @MathisGD So all good to merge this?

MathisGD commented 1 year ago

I would have harmonized the two contracts (https://github.com/pentagonxyz/huffmate/pull/99#discussion_r1111801202) but that's not a big deal

AmadiMichael commented 1 year ago

I would have harmonized the two contracts (https://github.com/pentagonxyz/huffmate/pull/99#discussion_r1111801202) but that's not a big deal

Is the current commit okay?