feat(sidecar-extensibility): detect upgradeable contracts #237
Conversation
serichoi65
changed the title
serichoi65
force-pushed
the
seri.choi/upgradable-contracts
branch
from
2b3b4c7 to
c05e33b
Compare
serichoi65
force-pushed
the
seri.choi/upgradable-contracts
branch
from
c05e33b to
d91781e
Compare
| zap.String("proxyContractAddress", proxyContractAddress), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd simplify this a bit:
if err != nil {
cm.Logger.Sugar().Errorw("Failed to create proxy contract",
zap.Error(err),
zap.String("contractAddress", contractAddress),
zap.String("proxyContractAddress", proxyContractAddress),
)
return nil, err
}
if found {
cm.Logger.Sugar().Debugw("Found existing proxy contract",
zap.String("contractAddress", contractAddress),
zap.String("proxyContractAddress", proxyContractAddress),
)
return proxyContract, nil
}
cm.Logger.Sugar().Debugw("Created proxy contract",
zap.String("contractAddress", contractAddress),
zap.String("proxyContractAddress", proxyContractAddress),
)That said, I would actually avoid using theFindOrCreateProxyContract at all and break this into two calls:
- Get the proxy contract by implementation address and its proxy. If it exists, return it
- Explicitly create it once you have all of the data to do so
In blocklake we did it the way you have it here because we had the ability to backfill and retry. In the sidecar, we dont have that luxury, so rather than upsert the proxy with only half the data, then update it with more info, but then potentially deal with having to purge it later since it can only half exist, we want to make a single atomic insert.
| } | ||
| } | ||
|
|
||
| bytecode, err := cm.EthereumClient.GetCode(context.Background(), proxyContractAddress) |
There was a problem hiding this comment.
If we need to take a context object, we should have CreateProxyContract take it as an argument and pass it along
| zap.String("bytecodeHash", bytecodeHash), | ||
| ) | ||
|
|
||
| _, _, err = cm.ContractStore.FindOrCreateContract( |
There was a problem hiding this comment.
Like I said above, we should refactor this whole create function to create the entire proxy contract in one call rather than upserting it, fetching data, then updating it.
|
|
||
| func (f *Fetcher) GetContractStorageSlot(ctx context.Context, contractAddress string, blockNumber uint64) (string, error) { | ||
| stringBlock := "" | ||
| if blockNumber == 0 { |
There was a problem hiding this comment.
Since we process everything block by block, we want this to explicitly be the block height we're processing, i.e. the provided blockHeight argument
There was a problem hiding this comment.
If it's 0, we should return an error
| @@ -156,6 +157,55 @@ func (idx *Indexer) ParseInterestingTransactionsAndLogs(ctx context.Context, fet | |||
| return parsedTransactions, nil | |||
| } | |||
|
|
|||
| func (idx *Indexer) IndexContractUpgrade(ctx context.Context, blockNumber uint64, upgradedLog *parser.DecodedLog) error { | |||
There was a problem hiding this comment.
Rather than this living in the Indexer this should probably be in the contract manager and be called something like HandleContractUpgrade
| t.Fatal(err) | ||
| } | ||
|
|
||
| baseUrl := "https://winter-white-crater.ethereum-holesky.quiknode.pro/1b1d75c4ada73b7ad98e1488880649d4ea637733/" |
There was a problem hiding this comment.
We should probably make this a node that isnt one of our paid quiknode endpoints
|
|
||
| t.Run("Test indexing contract upgrades", func(t *testing.T) { | ||
| // Create a contract | ||
| _, found, err := contractStore.FindOrCreateContract(contract.ContractAddress, contract.ContractAbi, contract.Verified, contract.BytecodeHash, contract.MatchingContractAddress, false) |
There was a problem hiding this comment.
This is an okay way to do this. IMO I would create the models directly in the db with grm.Model().Create(), main reason being it saves you (and anyone else) from needing to update this later if FindOrCreateContract changes for some reason.
| { | ||
| Name: "implementation", | ||
| Type: "address", | ||
| Value: common.HexToAddress("0x789"), |
There was a problem hiding this comment.
Would recommend using a valid address here just to make it as complete as possible
| res = grm.Raw(`select count(*) from proxy_contracts where contract_address=@contractAddress`, sql.Named("contractAddress", contractAddress)).Scan(&proxyContractCount) | ||
| assert.Nil(t, res.Error) | ||
| assert.Equal(t, 2, proxyContractCount) | ||
| }) |
There was a problem hiding this comment.
Probably need a test as well to cover getting the address from the storage slot
|
Hi seri - great PR! Can we have a description please? |
|
@serichoi65 I'll let you fill in the PR body with implementation specifics. @non-fungible-nelson I added a link to the related github issue this addresses |
[WIP with PR description]
This PR includes two features:
References: #223