search

cartesi / rollups-contracts

Smart Contracts for Cartesi Rollups
https://cartesi.github.io/rollups-contracts/
Apache License 2.0
17 stars 37 forks source link

safe erc20 delegate #271

Closed ZzzzHui closed 2 months ago

ZzzzHui commented 2 months ago

I call it delegate and create the folder delegate. Let me know if you think it should be named something else

guidanoli commented 2 months ago

Why not make it a library? It won't be able to access its storage anyway when called by the application contract via DELEGATECALL.

guidanoli commented 2 months ago

I'd also rename it to something like LibERC20Transfer

ZzzzHui commented 2 months ago

Why not make it a library? It won't be able to access its storage anyway when called by the application contract via DELEGATECALL.

It's basically the same in this case to be a contract and library. But library is indeed a better name, so I will change contract to library.

guidanoli commented 2 months ago

It's basically the same in this case to be a contract and library. But library is indeed a better name, so I will change contract to library.

It's not just about naming. In Solidity, they have different semantics. For example, state variables cannot be declared. This avoids programming errors. So, if the contract is going to be called via DELEGATECALL, it's good practice to declare it as a library.

ZzzzHui commented 2 months ago

I think we should keep these delegate libraries in a separate folder, rather than mixing together with existing internal libraries. I think we could put them in the folder called delegate. What do you think?

guidanoli commented 2 months ago

I think we should keep these delegate libraries in a separate folder, rather than mixing together with existing internal libraries. I think we could put them in the folder called delegate. What do you think?

The word "delegate" can be confusing for those familiar with "delegated proof-of-staking". Maybe delegatecall?

ZzzzHui commented 2 months ago

I think we should keep these delegate libraries in a separate folder, rather than mixing together with existing internal libraries. I think we could put them in the folder called delegate. What do you think?

The word "delegate" can be confusing for those familiar with "delegated proof-of-staking". Maybe delegatecall?

Yes. delegatecall will work.

Also, would be nice, if possible, to add test cases to Application.t.sol to test the DELEGATECALL voucher feature. :-)

I agree that would be better.