Add `ERC20_approveFrom`

[CalabashSquash]

🧐 Motivation The solidity implementation of ERC20 has an _approve function, which is an internal function for which the external approve function calls after fetching msg.sender.

Having an internal approval function that not only takes the spender and amount, but also owner allows for further extensions to the ERC20 contracts without having to duplicate much of the approve function body.

For example, ERC20Permit requires this when granting permission based on the EIP712 signatures.

I was trying to implement ERC20Permit in Cairo as a personal learning exercise but realised I would have to re-write much of ERC20_approve to allow for me to approve with a given owner address.

📝 Details

• Add a function named ERC20_approveFrom, which takes the parameters owner : felt, spender : felt, amount : Uint256, which modifies the ERC20_allowances mapping of the owner address parameter's allowance for the spender address parameter.

  • Also checks for zero addresses, etc.

• Modify the ERC20_approve function to look something like the following (not tested):

  
  func ERC20_approve{
      syscall_ptr : felt*, 
      pedersen_ptr : HashBuiltin*,
      range_check_ptr
  }(spender: felt, amount: Uint256):

• with_attr error_message("ERC20: amount is not a valid Uint256"):

• uint256_check(amount)

• end

  let (caller) = get_caller_address()

• with_attr error_message("ERC20: zero address cannot approve"):

• assert_not_zero(caller)

• end

• with_attr error_message("ERC20: cannot approve to the zero address"):

• assert_not_zero(spender)

• end

• ERC20_allowances.write(caller, spender, amount)

• Approval.emit(caller, spender, amount)

• ERC20_approveFrom(caller, spender, amount) # This function now does basically everything that has been deleted from ERC20_approve return () end

In the solidity implementation, the internal function is prepended with an underscore. This doesn't rely make sense here because of the ERC20_[functionName] format for functions, so I think ERC20_approveFrom is a suitable function name.

I'd like to implement this, but let me know whether this is wanted or not :)

[andrew-fleming]

Hey @CalabashSquash, I totally agree with your points! There's already a PR with a similar change in place #240. It might be a good idea to add the changes from said PR and build off that with permit. I should also note that we're refactoring the libs to utilize namespace (see discussion #267).

[andrew-fleming]

Also, I know you said this is a learning exercise, but EIP712 could be a cool feature to add as an issue/feature request. If it makes sense, it'd be great if you wanted to contribute :)

[martriay]

Superseded by #240