Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Handling DiamondProxy in autoload: Respecting followProxies: false #132

Open
yohamta opened this issue Sep 30, 2024 · 2 comments
Labels
bug Something isn't working maybe Not sure if this is a good idea yet soon Planned, targeting a near-future release

Comments

@yohamta
Copy link
Contributor

yohamta commented Sep 30, 2024

Proposal

Currently, when using autoload with DiamondProxy, facet addresses and ABIs are resolved even when followProxies: false is set. This behavior might not align with the expected use case when users want to retrieve proxies and implementations separately.

Options

  • Option 1: For DiamondProxy, do not resolve the implementation ABIs when followProxies: false is set.
  • Option 2: Add an isProxyResolved flag to AutoloadResult to indicate whether the proxy has been resolved (e.g., when the ABI for the implementation address has been resolved in cases like DiamondProxy, this flag would be set to true).
  • Option 3: When autoload resolves the implementation ABI, return an empty proxies field in AutoloadResult.

I'm not entirely sure which option would be the most suitable at the moment. What do you think?

@shazow
Copy link
Owner

shazow commented Sep 30, 2024

Ah I think you're right, this is a bug. If followProxies: false, then the DiamondProxy resolving should be bundled into the result.followProxies() callback.

I'll try that fix, but there might have been a reason I didn't do that in the first place, let's find out! :)

@shazow shazow added bug Something isn't working urgent Blocker for next release labels Sep 30, 2024
@shazow shazow closed this as completed in bb9f2c9 Oct 1, 2024
@shazow shazow reopened this Oct 1, 2024
@shazow shazow added soon Planned, targeting a near-future release maybe Not sure if this is a good idea yet and removed urgent Blocker for next release labels Oct 1, 2024
@shazow
Copy link
Owner

shazow commented Oct 1, 2024

This was undone in fe8fa90 since we weren't sure what behaviour we want yet.

Also might be a good excuse to refactor autoload into more modular pieces to avoid recursing with repeated getCode.

Punting it to a future release, so we can get the current changes published. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working maybe Not sure if this is a good idea yet soon Planned, targeting a near-future release
Projects
None yet
Development

No branches or pull requests

2 participants