-
Notifications
You must be signed in to change notification settings - Fork 1
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(sidecar-extensibility): detect upgradeable contracts and pull ABIs from IPFS #237
base: master
Are you sure you want to change the base?
Conversation
c05e33b
to
d91781e
Compare
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 |
8c7f61d
to
dd4f27c
Compare
pkg/abiFetcher/abiFetcher.go
Outdated
// Convert to base58 | ||
base58Hash := base58.Encode(bytes) | ||
|
||
return fmt.Sprintf("https://ipfs.io/ipfs/%s", base58Hash), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should definitely be taken as a config value that can be overridden at runtime.
pkg/abiFetcher/abiFetcher.go
Outdated
) | ||
fmt.Printf("IPFS URL: %s \n", url) | ||
|
||
httpClient := &http.Client{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make things testable, an http client reference should probably be passed to the NewAbiFetcher. That way you could pass in a mocked client.
pkg/abiFetcher/abiFetcher.go
Outdated
} | ||
} | ||
|
||
func (af *AbiFetcher) FetchMetadataFromAddress(ctx context.Context, address string) (string, string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming is a little funky since this returns the ABI not the metadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It returns bytecodeHash
and ABI
that I called it metadata, but do you prefer it to be FetchABIFromAddress
?
) | ||
fmt.Printf("bytecodeHash: %s", bytecodeHash) | ||
|
||
// fetch ABI using IPFS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we want to add Etherscan later (or even other options), I would refactor this a little bit into two components:
type AbiFetcher interface {
GetContractAbiAndMetadata(ctx context.Context, address string) (string, error)
}
type IAbiSource interface {
FetchAbi(ctx context.Context, address string, bytecode string) (string, error)
}
Move all of the IPFS logic into an implementation of IAbiSource
and have the current AbiFetcher
take a list of IAbiSource
s when created. Then all you need to do is iterate over the list in order until you find an ABI or error out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll refactor this in the PR when I add Etherscan
9e681ad
to
9fbc15f
Compare
In order to side-load contracts to Sidecar, we want to add the following features:
Non-feature updates:
AbiFetcher
as a new input toContractManager
pipelineIntegration_test
andrestakedStrategies_test
, should use a non-paid quiknode url addressNewClient
parameter withhttp.Client
to accept mocked clientReferences: #223