WIP: Adapters package (branches off of features/eth-tx-sending-adapter) #6
Conversation
Signed-off-by: Dimitri Roche <dimroc@gmail.com>
Signed-off-by: Steve Ellis <email@steveell.is>
Signed-off-by: Steve Ellis <email@steveell.is>
- rename adapter files based on functionality instead of type Signed-off-by: Dimitri Roche <dimroc@gmail.com>
There was a problem hiding this comment.
Everything looks reasonable to me. I would just recap those earlier two points:
- Should
configbe nested inservices? My initial reaction was no. - Where are you actually planning to use the config.EthereumIp? I assumed in EthSendTx but wanted to make sure. I always push the outside in as you know, but maybe it doesnt exist yet.
- Are we sure we want it on
configand not as a default parameter on the task? I think yes we want it on config, because it would be great to run a suite of jobs against test and then on production without reconfiguring the jobs and tasks.
adapters/eth_tx.go
Outdated
| import ( | ||
| "github.com/ethereum/go-ethereum/rpc" | ||
| "github.com/smartcontractkit/chainlink-go/models" | ||
| cf "github.com/smartcontractkit/chainlink-go/services/config" |
There was a problem hiding this comment.
Not sure how I feel about config being nested in services
There was a problem hiding this comment.
I'm happy to move it, just kind of left it where it was. Seemed very small for a package, but that's probably not a bad thing.
adapters/eth_tx.go
Outdated
| } | ||
|
|
||
| func (self *EthSendTx) Perform(input models.RunResult, config cf.Config) models.RunResult { | ||
| eth, err := rpc.Dial("http://example.com/api") |
There was a problem hiding this comment.
Is this where we would use config? I assume on deployment, there is only one ethereum ip to dial, unless you're going against test. Then putting it on config is fine. Another idea is to just make it a default but settable parameter on the EthSendTx node rather than config.
There was a problem hiding this comment.
Currently leaning towards putting config as an attribute on each adapter, and passing it in on initialization of each adapter.
There was a problem hiding this comment.
I'm concerned how this will affect serialization. Will it always try to write the config or can we omit it? For now you could just leave it as is on Perform and we could revisit that part later after the other parts of this refactor gets committed.
|
I don't think the Ethereum URL should be required as a parameter on EthSendTx, although making it overridable makes sense. Having an Ethereum node to talk to is a pretty hard requirement for ChainLink to operate and as long as the transaction is submitted, users shouldn't care which Ethereum node originally submitted it to the network. I was also thinking about making the Ethereum client( |
|
Also, to answer your second question: we use the Ethereum URL whenever dealing with the Ethereum network. The two big use cases there are submitting transactions and watching for event logs, but both of those will be used in various ways. |
|
Hmm, on initialization. Have a few mixed thoughts on it so will comment on
it in the morning. I’m just in love w how lean our current adapter scenario
is. Should be fine.
…On Wed, Dec 13, 2017 at 6:46 PM Steve Ellis ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In adapters/eth_tx.go
<#6 (comment)>
:
> @@ -0,0 +1,26 @@
+package adapters
+
+import (
+ "github.com/ethereum/go-ethereum/rpc"
+ "github.com/smartcontractkit/chainlink-go/models"
+ cf "github.com/smartcontractkit/chainlink-go/services/config
"
+)
+
+type EthSendTx struct {
+ Address string `json:"address"`
+ FunctionID string `json:"functionID"`
+}
+
+func (self *EthSendTx) Perform(input models.RunResult, config cf.Config) models.RunResult {
+ eth, err := rpc.Dial("http://example.com/api")
Currently leaning towards putting config as an attribute on each adapter,
and passing it in on initialization of each adapter.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#6 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAmw8TGmXMTJ5T_wQHtghiMknODLQG2pks5tAGHkgaJpZM4RAJ5h>
.
|
se3000
force-pushed
the
adapters-package
branch
from
a10c7d6 to
a56f73c
Compare
# This is the 1st commit message: update contract # This is the commit message #2: make scripts executable # This is the commit message #3: upload base too # This is the commit message #4: try again # This is the commit message #5: gather changesets, print products out of scope # This is the commit message #6: fix product finding # This is the commit message #7: try again # This is the commit message #8: debug # This is the commit message #9: debug 2 # This is the commit message #10: grab only modified or added changesets # This is the commit message #11: try again 1 # This is the commit message #12: remove early exit # This is the commit message #13: place changesets in a subfolder in final artifact # This is the commit message #14: validate whether everything was generated
# This is the 1st commit message: fetch whole repo # This is the commit message #2: add comment # This is the commit message #3: fix paths for validation # This is the commit message #4: try with functions # This is the commit message #5: fix spelling # This is the commit message #6: display failed UML generation once
Previously models had a dependency on adapters, but that seems backwards. Adapters are like mini "services" and may need to persist things. On the other hand, models seem to be primarily focused on persisting data, and so shouldn't have to know about the application logic in adapters.
This moves adapters into it's own package, and makes it so that the adapters package has a dependency on models. This is a little weird with validations(see the bottom of adapters/adapter.go), but for the most part seems like the right direction for the dependencies to point.
Currently in the middle of moving the config out of the serivces package, as adapters need to know about services(specifically config). If adapters depend on services then there is a circular dependency (services -> adapters -> services). Moving config out into its own package allows for services and adapters to both depend on config, and services can still depend on adapters.