Consolidate configs for the EVM chain client for easier external use #12294
Conversation
|
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
… out of legacyevm
amit-momin
force-pushed
the
poc-atlas-chain-client-2
branch
from
333ad89 to
f040413
Compare
There was a problem hiding this comment.
Are there plans to reuse this function for Atlas? If not I don't see any point to separate it from chain.go. We should try to avoid the existing pattern we've created with builder files. It's bad for readability and reader has to understand why there are so many files with similar names around.chain.go serves the purpose of a builder file on it's own anyway.
There was a problem hiding this comment.
That's the plan and even beyond Atlas hopefully if any other external component needs to eventually integrate. I wanted to minimize the number of imports they would need to do to use our chain client and also didn't want to ask them to import one that has "legacy" in the name if this is a long term solution. I could move this into an existing file but kept it separate thinking a shorter file was better.
core/chains/evm/client/models.go
Outdated
There was a problem hiding this comment.
Not a big fan of adding aliases unless they are needed repeatedly. For example EvmMultiNode is only used once and we introduced a new file and a new alias for the user to remember.
There was a problem hiding this comment.
I was thinking this was ok since it follows the same convention we use in the txmgr package. I could revert EvmMultiNode, but if we keep the file for the EvmNode and EvmSendOnlyNode aliases, I think it'd be better to have all of the type parameters we're using for generics in a centralized place for reference. That being said if it's still better to avoid aliases, I can revert these back and delete this file.
There was a problem hiding this comment.
We introduced many aliases back when txm was generalized and now I often find myself looking around for files with similar names trying to figure out where is what. Personally, unless an alias helps with verbosity I avoid it as it adds another layer of abstraction for the reader to remember. But it really depends on the situation, so if you feel this is going to be used in Atlas as well, no problem with keeping it.
There was a problem hiding this comment.
In that case I'll revert these. Atlas or external components shouldn't need them. I'm trying to position the code in a way they would only need basic go types. This was more so for cleaning up internally since some of them written out with type arguments looked like clutter. It also required redefining the parameters in multiple places. For this PR, it's better I leave this as is. If we ever want to clean things up a certain way, we can do that as a separate effort.
There was a problem hiding this comment.
Just reverted in the latest commit. Should be good for another look.
There was a problem hiding this comment.
AFAIK it's not ideal to reuse names already used in package's path. Here we have .../evm/client/evm_client and .../evm/client/evm_client_config. It's already in a bad state since we have to maintain two types of clients (client and chain_client due to the deprecation process), I'd like us to try and simplify things. Maybe a simple config.go or config_builder.go sounds better.
There was a problem hiding this comment.
Good to know this best practice for the future! Updated the file name to config_builder.go.
|
…12294) * Consolidated configs for easier external use and moved the evm client out of legacyevm * Reused existing config types and created centralized models for client types * Removed deprecated client constructor * Renamed the client config builder file * Added evm client and config builder tests * Fixed linting * Reverted client type aliases
legacyevmpackage