Add API-level proxy detection and implementation resolution #1711
Conversation
manuelwedler
changed the title
|
Can you rebase this @manuelwedler I am seeing changes from #1689 |
| } = {}; | ||
| if ( | ||
| req.query.resolveProxies === "true" && | ||
| found[0].onchainRuntimeBytecode |
There was a problem hiding this comment.
Just to make sure, here we use the onchainRuntimeBytecode from the Database right? Not fresh from an RPC
| chainId, | ||
| status: found[0].runtimeMatch, | ||
| ...proxyStatus, | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| // ignore |
There was a problem hiding this comment.
Not introduced by this PR but not a good idea to ignore silently. We should log here
manuelwedler
force-pushed
the
proxy-resolution
branch
from
ef2313b
to
fc49f87
Compare
|
Done. I think this was due to rewriting history. IMO, we should never do this again. |
Right, but not never, IMO it was necessary in this case, unfortunately. |
There was a problem hiding this comment.
I don't fully understand what the code here does in detectAndResolveProxy. Some explanation and comments would help.
| signatureLookup: false, | ||
| followProxies: false, | ||
| }); | ||
| const proxies = detectionResult.proxies; |
There was a problem hiding this comment.
For me it's not clear what we are doing after this. We have the proxies. Now what? Why are we doing the steps below?
| proxyType: "EIP1167Proxy", | ||
| implementations: [fixedProxy.resolvedAddress], | ||
| }; | ||
| } // Ignore FixedProxy otherwise because it could be a false-positive |
| const checkedProxyTypes: Set<ProxyType> = new Set([ | ||
| "FixedProxy", | ||
| "DiamondProxy", | ||
| ]); // Needed because of potential duplicates |
There was a problem hiding this comment.
Also didn't get the potential duplicates issue here
| "FixedProxy", | ||
| "DiamondProxy", | ||
| ]); // Needed because of potential duplicates | ||
| for (const proxy of proxies) { |
There was a problem hiding this comment.
Above I see we handle the EIP1167 and DiamondProxy cases. Is this here the rest of the cases for other ProxyTypes?
| sourcifyChain, | ||
| ); | ||
|
|
||
| const implementations = await Promise.all( |
There was a problem hiding this comment.
| const implementations = await Promise.all( | |
| // Find contract names if the implementations are verified on Sourcify | |
| const implementations = await Promise.all( |
Closes #1655
This implements an API-level proxy detection as in #1655 by making use of the whatsabi library. A few explanations about this PR:
SourcifyChainclass for making it compatible to the whatsabi library as a provider, and for being able to resolve the implementation addresses ofDiamondProxycontracts.proxyResolutionErrorto the API response for the case that the proxy resolution fails. This could be due to an unresponsive RPC for example. In such a case, I still want to respond successfully to the user but let them know why the proxy fields are not there.FixedProxyResolverwas returned in a lot of cases where actually a library was called by aDELEGATECALL. I decided to only support EIP1167 proxies for this resolver. Any otherFixedProxywould be a non-standardised proxy (or even a library). So in case aFixedProxyResolveris detected, we check explicitly for the opcodes standardised in EIP1167.0x0. The latter was for example the case for a factory contract which deploys EIP1967 contracts. Therefore, I iterate over the returned resolvers and return the first implementation address which is not0x0./check-all-by-addressesendpoint and not thecheck-by-addressesendpoint, because in the latter thechainIdsarray does not contain objects.Remaining issue (non-blocking)
There is still a problem left from shazow/whatsabi#142:
A contract that embeds a proxy (as for example a factory contract) could falsely be detected as a proxy.
A first proposal to solve this is to not consider contracts with
CREATE/CREATE2opcodes as proxies. However, this will not catch all contracts that only embed proxy bytecodes, and also exclude factory contracts that could be at the same time a proxy.Edit: We decided that this problem is not a blocker for this PR. IMO, it is good to get this out and test it. The proxy data won't be persisted in our database with this change, so before we persist it we might be able to improve the proxy detection.