OpenZeppelin / cairo-contracts

OpenZeppelin Contracts written in Cairo for Starknet, a decentralized ZK Rollup
https://docs.openzeppelin.com/contracts-cairo
MIT License
823 stars 335 forks source link

erc20/library.cairo "Transfer" event param "_from" not compatible with Starknet Python testing library #219

Closed eddiexbank closed 2 years ago

eddiexbank commented 2 years ago

📝 Details

with Starkware official testing Python package: starkware/starknet/testing/contract_utils.py line 100, it encodes processed StarknetTransaction with namedtuple. Thus, calling erc20 Transfer with current OZ event signature

@event
func Transfer(_from : felt, to : felt, value : Uint256):
end

will fail with ValueError: Field names cannot start with an underscore: '_from'

🔢 Code to reproduce bug

In Python, try calling Transfer with this pattern tx = await erc20_contract.transfer(10, dst.contract_address).invoke(src) given

martriay commented 2 years ago

Mhh I see. I honestly always hated how this ended up looking. @bbrandtom do you think Cairo can release the from keyword and make it available? It's a very useful one for smart contract programming 😓.

Otherwise, maybe we can change it to from_ which isn't the best but...

milancermak commented 2 years ago

I don't think from can be made available, it's used for importing, e.g. from starkware.cairo.common.alloc import alloc.

martriay commented 2 years ago

Mhhhh right

On Wed, Mar 16, 2022 at 8:55 AM Milan Cermak @.***> wrote:

I don't think from can be made available, it's used for importing, e.g. from starkware.cairo.common.alloc import alloc.

— Reply to this email directly, view it on GitHub https://github.com/OpenZeppelin/cairo-contracts/issues/219#issuecomment-1069157994, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAM77FZ24JXDL6VCV5IAOLDVAHR6TANCNFSM5QUXRQ6Q . You are receiving this because you commented.Message ID: @.***>

andrew-fleming commented 2 years ago

Is From completely out of the question?

func Transfer(From : felt, to : felt, value : Uint256):
end

vs

func Transfer(from_ : felt, to : felt, value : Uint256):
end
milancermak commented 2 years ago

To me, that looks too much like a type name (when compared to Uint256), but that's just my opinion.

andrew-fleming commented 2 years ago

Yeah, that's fair. I suppose ugly is better than potentially confusing.

martriay commented 2 years ago

Yeah, that's fair. I suppose ugly is better than potentially confusing.

Agree. Luckily argument names are not part of Ethereum's ABI specification, so it shouldn't break anything. I'm unsure about the events though.

Let's go with from_ then.

eddiexbank commented 2 years ago

Great!