Inherit asset decimals in ERC4626

[Amxx]

Inherit the decimal value from the asset (if any) while still supporting customization on top of ERC4626

Fixes #3549

PR Checklist

• Tests
• Changelog entry

[Amxx]

The current tests already check decimal overriding.

We don't have a test where the fallback is done. Will work on that.

[Amxx]

Testing should be good now

[frangio]

This is an improvement over the status quo because it will work for tokens with no decimals. However, one of the points raised in #3549 was:

Even if both tokens expose decimals the method is for usability, not for onchain calculations.

I find this pretty compelling, but to stop using decimals in the conversion functions would be a breaking change now...

From #3549 (comment):

What are the _initialConvertToXxx for. They make the code more complexe, and I don't see a reason for that.

The reason for those functions was to define conversion without reference to the decimals values.

[Amxx]

Initial conversion function added. They are tests trough our current test suite (that checks decimals mismatch).

Need a breaking change description

[frangio]

I changed the variable name in the constructor which was assetDecimals, because in the catch branch those are expressly not the asset's decimals. If you agree with this let's merge.

[alexflorence]

this breaks upgradability from previous implementations to some extent because the initializer is not called on upgrading and therefore_decimals is never set. so decimals() needs to be overridden with a hardcoded value instead of calling super.decimals()

[frangio]

@alexflorence This is a good point we didn't notice. It's solved by calling the initializer though in a function like this:

function migrateToV48() public reinitializer(2) {
    __ERC4626_init(asset);
}

We should add a note in the changelog for this.

We can consider defaulting to super.decimals if the stored decimals are 0 too.

@alexflorence Let us know your thoughts.