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

feat(protocol): get rid of new compiler warnings #15613

Merged
merged 9 commits into from
Jan 31, 2024

Conversation

dantaik
Copy link
Contributor

@dantaik dantaik commented Jan 30, 2024

undone the implementation() function

Copy link

vercel bot commented Jan 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
bridge-ui-v2-a6 ✅ Ready (Inspect) Visit Preview Jan 30, 2024 3:08pm

@dantaik
Copy link
Contributor Author

dantaik commented Jan 30, 2024

I'm trying to figure out :
Screenshot 2024-01-30 at 20 29 56

@Brechtpd
Copy link
Contributor

Should be possible already using eth_getstorageat as explained in the comments: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/61117c4db8497ba489d5e1e127565a011ed6907a/contracts/proxy/ERC1967/ERC1967Proxy.sol#L33

image

I assume the reason not to expose it directly onchain is to lower the cost of deploying the proxy contract because public functions require extra opcodes. Even though in this case the deployment cost isn't really that important, seems like the method to get the implementation contract is already easy.

@dantaik
Copy link
Contributor Author

dantaik commented Jan 30, 2024

Should be possible already using eth_getstorageat as explained in the comments: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/61117c4db8497ba489d5e1e127565a011ed6907a/contracts/proxy/ERC1967/ERC1967Proxy.sol#L33

image

I assume the reason not to expose it directly onchain is to lower the cost of deploying the proxy contract because public functions require extra opcodes. Even though in this case the deployment cost isn't really that important, seems like the method to get the implementation contract is already easy.

With this approach, the dapp/client side then has no way of reading the impl to customize some logics then.

@dantaik
Copy link
Contributor Author

dantaik commented Jan 30, 2024

I'm trying to figure out : Screenshot 2024-01-30 at 20 29 56

I have to make this change to get rid of the compiler warnings: f36f5ab

Any idea?

@davidtaikocha davidtaikocha added this pull request to the merge queue Jan 31, 2024
Merged via the queue into alpha-6 with commit ccee985 Jan 31, 2024
14 checks passed
@davidtaikocha davidtaikocha deleted the expose_impl_addr branch January 31, 2024 01:06
@dantaik dantaik restored the expose_impl_addr branch January 31, 2024 01:09
@dantaik dantaik deleted the expose_impl_addr branch January 31, 2024 01:10
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.

4 participants