Add tezosconnector support #261
Conversation
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
Signed-off-by: Dzianis Andreyenka <andreenkodn@gmail.com>
denisandreenko
requested review from
nguyer,
awrichar and
peterbroadhurst
as code owners
There was a problem hiding this comment.
This is a great start and will get us to an easy way to get Tezosconnect up and running! I have some questions around how we want to do End-To-End tests for FF + Tezosconnect. Typically that's involved setting up a local dev chain and running FF connected to it, but I'm not sure what our options are there for Tezos. That's for the next PR though :)
| type Connector interface { | ||
| GetServiceDefinitions(s *types.Stack, dependentServices map[string]string) []*docker.ServiceDefinition | ||
| GenerateConfig(stack *types.Stack, member *types.Organization) Config | ||
| Name() string | ||
| Port() int | ||
| } |
There was a problem hiding this comment.
Since there is only one type of Tezos connctor (tezosconnect) and one type of Tezos implementation, I don't know that we need this interface. The reason it exists for Ethereum is that there are different Ethereum implementations that are set up very differently (Besu, Geth, etc)
There was a problem hiding this comment.
I thought about it. But I still think it's good practice to use interfaces, even if we currently have 1 implementation.
This does not complicate the code, but at the same time gives additional flexibility.
There was a problem hiding this comment.
I'm fine with leaving it. In general I'm not a fan of just adding extra interfaces unless 1) It is necessary for unit testing 2) We are certain that there will be another implementation in the future. I know it is common practice in other languages to use interfaces and abstraction absolutely everywhere but I have always disliked that pattern. But I recognize this is a personal preference. So I'm fine with this as is.
There was a problem hiding this comment.
Thank you for sharing your opinion @nguyer !
| serviceDefinitions[i] = &docker.ServiceDefinition{ | ||
| ServiceName: "tezosconnect_" + member.ID, | ||
| Service: &docker.Service{ |
There was a problem hiding this comment.
Do we also need to set up a signer service here?
There was a problem hiding this comment.
The thing is that the signatory service works with many different kms and everyone should decide which one they prefer.
That's why there is no one in the FF CLI.
The idea is that user run its own service for tx signing using chosen kms and after that uses FF CLI with the '--connector-config' and provided address of this service.
Configuration example:
connector:
blockchain:
signatory: http://127.0.0.1:6732
There was a problem hiding this comment.
In the future, there is an idea to make a kind of local signing of transactions, something like FF signer.
But at the moment, there are other priorities.
There was a problem hiding this comment.
Okay. This is fine for now. I think when we go to create automated E2E tests we will need to revisit this though.
There was a problem hiding this comment.
Of course.
In fact, the idea of setting up the Signature as part of the FF stack is not that bad,
because by default we can use local keys for tx signing (similar to FF signer).
This is not super secure and for production KMS should be used (or HSM),
but for local use it is a very good option
No description provided.