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.

Sign up for GitHub

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

Jump to bottom

Add sentinel and implementation so that Etherscan can mark the ReflexEndpoint as a proxy #99

Open
zerosnacks opened this issue Jun 23, 2023 · 2 comments · May be fixed by #113
Open

Add sentinel and implementation so that Etherscan can mark the ReflexEndpoint as a proxy #99

zerosnacks opened this issue Jun 23, 2023 · 2 comments · May be fixed by #113
Labels
discussion help wanted Extra attention is needed

Comments

@zerosnacks
Copy link
Contributor

zerosnacks commented Jun 23, 2023 •
edited
Loading

Implemented a draft in #113, pending further discussion with implementers.

Goals:

  • Get Etherscan to recognize the Endpoint as a valid proxy
  • Optimize the proxy to be written in bytecode and insert the DELEGATECALL opcode somewhere in the callstack
  • Avoid function selector clashing of sentinel and implementation

The current sentinel as proposed in the draft is far from ideal but works as intended.

Etherscan needs two features to mark it as a proxy:

  • contains a delegatecall(), even if unreachable
  • contains an implementation() which resolves to the implementation

The benefit of having this is that users can interact with the endpoint using the ABI of the implementation.
@JLongarzo
Copy link
Contributor

An ideal world is obviously one where Etherscan just supports Reflex... but they are slow. Up to you I'll point out a few tradeoffs with this decision.

  1. More perifery/endpoint implementations increases audit costs for Reflex and implementers.
  2. Increases complexity and dev overhead for implementers who want to support etherscan.
  3. Implementers might decide not to support etherscan without fully understanding the ramifications. They could then regret this later and be unable to change it.
  4. Having any Reflex implementations not being supported on Etherscan could reflect negatively on the framework and its brad.

@zerosnacks zerosnacks mentioned this issue Jun 27, 2023
Merged
6 tasks
@zerosnacks
Copy link
Contributor Author

zerosnacks commented Jul 4, 2023 •
edited
Loading

An ideal world is obviously one where Etherscan just supports Reflex... but they are slow. Up to you I'll point out a few tradeoffs with this decision.

  1. More perifery/endpoint implementations increases audit costs for Reflex and implementers.

True, I think having this as a known issue the moment we launch so we can make it a topic of discussion with Etherscan (pointed to the draft PR) and also accepting external contributions to improve the workaround we have now is the way to go I think.

  1. Increases complexity and dev overhead for implementers who want to support etherscan.

There are some problems are function selector shadowing of implementation and sentinel which users have to be aware of.

  1. Implementers might decide not to support etherscan without fully understanding the ramifications.
    They could then regret this later and be unable to change it.

Ideally before we get it audited an have an official 1.0.0 release we have a better solution or we decide with actual implementers how to go about this.

  1. Having any Reflex implementations not being supported on Etherscan could reflect negatively on the framework and its brad.

I agree but we have a proposed workaround now

I've added a draft PR here: #113

@zerosnacks zerosnacks changed the title Remove sentinel, move into draft PR Add sentinel and implementation so that Etherscan can mark the ReflexEndpoint as a proxy. Jul 4, 2023
@zerosnacks zerosnacks changed the title Add sentinel and implementation so that Etherscan can mark the ReflexEndpoint as a proxy. Add sentinel and implementation so that Etherscan can mark the ReflexEndpoint as a proxy Jul 4, 2023
This was referenced Jul 4, 2023
Closed
Closed
@zerosnacks zerosnacks added the help wanted Extra attention is needed label Jul 4, 2023
@zerosnacks zerosnacks linked a pull request Jul 4, 2023 that will close this issue
Draft
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
discussion help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add Endpoint sentinel for proxy recognition by Etherscan chromaxyz/reflex
2 participants
@JLongarzo @zerosnacks