Make AToken's initialize and _transfer functions virtual so they can be extended

[davidlaprade]

This is a follow up to #726

It does two things:

• makes aToken.intialize public and virtual
• makes aToken._transfer virtual

Doing so will make AToken easier to extend, e.g. for grant-funded projects like the one I'm working on.

I'd like initialize() to be public as well so that the override can just call super.initialize(...) to inherit all the current functionality without having to copy/paste it.

[miguelmtzinf]

Thanks for the suggestion @davidlaprade . It is a bit challenging to identify/pick which functions should be virtual or not. It's a matter to find a golden balance, between having all functions as virtual or not having any.

At the end of the day, the conclusion was to have the functions of the ScaledBalanceToken as virtual, while leaving the AToken as it is. This is because in the case of creating a custom AToken you just need to either inherit the AToken contract and add a couple of tweaks (to mint/burn functions maybe) or inherit ScaledBalanceToken and recreate your AToken in your own.

How does it sound? If adding this change, we should re-evaluate which other functions of the AToken contract need to be virtual too, and set up a strategy about this.

[davidlaprade]

That's good feedback, thanks @miguelmtzinf!

It is a bit challenging to identify/pick which functions should be virtual or not. It's a matter to find a golden balance, between having all functions as virtual or not having any.

Agreed, and I can totally see why you'd want to follow a principled strategy rather than just making functions virtual at the request of rando's on the internet 😅

in the case of creating a custom AToken you just need to either inherit the AToken contract and add a couple of tweaks (to mint/burn functions maybe) or inherit ScaledBalanceToken and recreate your AToken in your own

As I think about this more, I think it makes sense to not make the aToken.initialize function overrideable, since that function is called by a privileged account when assets are added to the reserve. If we want to do anything post-initialization, that can be done if/when an aToken's implementation is upgraded after it has already been added. That way Aave inherits less 3rd party risk. So I'll remove 8865203 from this PR.

With respect to _transfer(address, address, uint128, bool), however, I think that making it virtual would better follow your existing conventions.

There are 3 core balance-altering ERC20 functions: _mint, _burn, and _transfer. Of those three, _transfer is currently the only one that's not virtual (_mint, and _burn are). We're finding it's difficult to write a lightweight extension of AToken with only 2/3 of these.

That said, this PR only changes one of the two _transfer functions in AToken. We felt that _transfer(address, address, uint128, bool) is the more important of the two since _transfer(address, address, uint128) just delegates to the latter. But if you think it'd be more consistent, I have no issues making that function virtual as well.

[mds1]

Hey @miguelmtzinf! Totally agree it can be challenging to figure out which functions should be virtual or not, but I have a few thoughts here so figured I'd chime in. (I'm working with @davidlaprade on the flexible voting AToken implementation).

I'll use our flexible voting extension as an example here, but this applies to any AToken market that extends the base AToken.sol functionality. I see three assumptions to consider when designing AToken.sol to accomodate extensions like this:

1. Inheriting from AToken is the simplest way to add functionality (as opposed to recreating an AToken implementation), as it avoids copy/paste errors and duplicating existing code. This also simplifies auditing the extension by avoiding the need to verify copy/pasted code.
2. These extensions may need additional initialization logic (in our case, it's self-delegating governance tokens).
3. These extensions may need to hook into token balance changes (in our case, it's checkpointing balances for voting tokens)

---

Given those first two assumptions, initialize feels like it should definitely be virtual. There's two options for deploying a new flex-voting-AToken market that requires custom initialization logic:

1. Make initialize virtual so the extension can implement additional logic. Then deploy the new market the standard way by passing the flex-voting-AToken implementation address and initialization calldata.
2. Keep initialize non-virtual. Deploy the market the standard way by passing the default AToken implementation address and initialization calldata. THEN you need to call upgradeAndCall with the new flex-voting-AToken address and more initialization data.

There's nothing inherently wrong with the second approach, it's just more costly and leaves more room for errors/mistakes. The first approach is cleaner and feels like the correct approach.

---

Given the first and third assumption, _transfer feels like it should also be virtual for the reasons @davidlaprade mentioned:

There are 3 core balance-altering ERC20 functions: _mint, _burn, and _transfer. Of those three, _transfer is currently the only one that's not virtual (_mint, and _burn are).

It seems the 4-arg _transfer is the one that should be virtual, as the 3-arg one delegates to that.

Curious to hear your thoughts!

[miguelmtzinf]

Your feedback is much appreciated @davidlaprade @mds1

Keep initialize non-virtual. Deploy the market the standard way by passing the default AToken implementation address and initialization calldata. THEN you need to call upgradeAndCall with the new flex-voting-AToken address and more initialization data.

As a workaround, you could just call initialize first, and setting the additional parameters via specific setters afterwards. However, I think that not having initialize virtual does not make much sense, because it goes against the usage of the params parameter. It's way better to have it virtual so custom AToken contracts can be made and use the decoding of the params accordingly.

Something like:

contract CustomAToken is AToken {

  function initialize(
    IPool initializingPool,
    address treasury,
    address underlyingAsset,
    IAaveIncentivesController incentivesController,
    uint8 aTokenDecimals,
    string calldata aTokenName,
    string calldata aTokenSymbol,
    bytes calldata params
  ) external override initializer {
    super.intialize(
      initializingPool,
      treasury,
      underlyingAsset,
      incentivesController,
      aTokenDecimals,
      aTokenName,
      aTokenSymbol,
      params
    );
    (
      address myAddress1,
      address myAddress2,
      uint256 myNumber1
    ) = abi.decode(params, (address, address, uint256));
    // do something

  }

It also makes sense to have the two transfer functions as virtual too. It feels more complete and consistent with the rest of the functions that can be potentially changed/extended.

Are you keen on tackling the changes please?

[mds1]

Awesome, thanks a lot @miguelmtzinf. We'll update this PR with the agreed upon changes to initialize and the two _transfer methods 🙌

[davidlaprade]

This needs to be public as well as virtual for extensions to be able to call super.initiailize(...)

[davidlaprade]

Thanks @miguelmtzinf @mds1!

I think that not having initialize virtual does not make much sense, because it goes against the usage of the params parameter. It's way better to have it virtual so custom AToken contracts can be made and use the decoding of the params accordingly.

Yeah, that's a great point. I agree.

Just made the changes, I think this is ready for review

[miguelmtzinf]

Thanks @davidlaprade @mds1 !