Skip to content

Conversation

@ericglau
Copy link
Member

@ericglau ericglau commented Nov 17, 2023

Fixes #925

Fixes upgrading a proxy from an implementation that has a fallback function and is not using OpenZeppelin Contracts 5.0. Also handles the case where a custom proxy admin may have a similar fallback function.

The fallback function causes the UPGRADE_INTERFACE_VERSION selector to return a value which can be anything, so if it returns anything other than "5.0.0", we should treat it as a pre-5.0 interface.

@ericglau ericglau requested a review from a team November 17, 2023 21:23
}

fallback(bytes calldata) external returns (bytes memory) {
return abi.encode(greeting);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it not work with a direct cast

Suggested change
return abi.encode(greeting);
return bytes(greeting);

?

Using abi.encode does an extra encapsulation. Not sure that is needed.

Copy link
Member Author

@ericglau ericglau Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually was needed to force the return type to look like a string in

(added a test in 521f1ac)

@ericglau ericglau requested a review from Amxx November 22, 2023 19:10
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor nits

throw new Error(`Unexpected type for UPGRADE_INTERFACE_VERSION at address ${address}. Expected a string`);
// Log as debug and return undefined if the interface version is not a string.
// Do not throw an error because this could be caused by a fallback function.
debug(`Unexpected type for UPGRADE_INTERFACE_VERSION at address ${address}. Expected a string`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest adding a test to make sure this is logged if possible although it's just debug data. Surely is something you'd like to see if you're doing a production deployment.

Copy link
Member Author

@ericglau ericglau Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this revealed that the tests were testing the wrong scenario, which is fixed in the latest two commits.

Added tests for the debug logging by using optional parameters for the relevant functions where we pass in a stub. It's not my preferred approach but couldn't get mocks to work well with this otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ignore UPGRADE_INTERFACE_VERSION if selector returns anything other than "5.0.0"

3 participants