[ETHEREUM-CONTRACTS] proposed changes for the macro forwarder

[d10r]

Proposed changes:

• rename TrustedMacrosVanilla to MacroForwarder. Reasoning: trusted is a property not inherent to the contract, but coming into effect by registering it as trusted by gov. Vanilla isn't self-explanatory, I find it unnecessary. If we add a 712 variant, we can simply add 712 to the name. Plural -> singular because the use of a variable trustedMacros for an instance in the test seemed odd.
• Removal of simulateMacro: Initially I tried to find a better name (_simulate_ to me suggests that it would simulate the actual execution, not just retrieve the operations), but I came to the conclusion that it's more confusing than helpful and should instead be removed. It doesn't really add anything, because it just delegates to macro.buildBatchOperations()`
• Added test case using StatefulMacro: [ETHEREUM-CONTRACTS] TrustedMacros: A TrustedForwarder that rules them all #1782 states: A user-defined macro is stateless ... - I think that's a mistake. The macro may very well have state if that helps to reduce the calldata size of the macro call. It just can't modify state in buildBatchOperations(). This test case exemplifies that, reducing the calldata to the minimum, which currently is the macro address.

[github-actions]

Changelog Reminder

Reminder to update the CHANGELOG.md for any of the modified packages in this PR.

• CHANGELOG.md modified
• Double check before merge

[hellwolf]

rename TrustedMacrosVanilla to MacroForwarder. Reasoning: trusted is a property not inherent to the contract, but coming into effect by registering it as trusted by gov. Vanilla isn't self-explanatory, I find it unnecessary. If we add a 712 variant, we can simply add 712 to the name. Plural -> singular because the use of a variable trustedMacros for an instance in the test seemed odd.

okay.

Removal of simulateMacro: Initially, I tried to find a better name (simulate to me suggests that it would simulate the actual execution, not just retrieve the operations), but I came to the conclusion that it's more confusing than helpful and should instead be removed. It doesn't really add anything, because it just delegates to macro.buildBatchOperations()`

I should probably still expose it, just for the merit of being able to get those batch operations call data. But it wouldn't work if it became stateful.

Added test case using StatefulMacro: #1782 states: A user-defined macro is stateless ... - I think that's a mistake. The macro may very well have state if that helps to reduce the calldata size of the macro call. It just can't modify state in buildBatchOperations(). This test case exemplifies that, reducing the calldata to the minimum, which currently is the macro address.

Not necessarily a mistake, but it was a conscious and defensive choice.

However, we can rethink here: was the defensiveness justified?

Let me think a bit.

[github-actions]

XKCD Comic Relif

Link: https://xkcd.com/1828