singnet / snet-cli

SingularityNET CLI for interacting with SNET blockchain contracts and deployed services.
MIT License
77 stars 52 forks source link

Add command to allow service provider claiming funds from channel #134

Closed vsbogd closed 5 years ago

vsbogd commented 5 years ago

At the moment channel claiming is implemented within snet-daemon through snet-daemon claim interface. This command should be moved into the snet-cli tool.

Possible way to implement it is to add additional gRPC interface to the daemon to:

This interface will be implemented by separate daemon instance: treasurer server.

Use this interface to implement snet-cli claim command.

See https://github.com/singnet/snet-cli/issues/134#issuecomment-443182697 for more detailed description

astroseger commented 5 years ago

There is a problem.

Which ethereum identity will be used? snet-cli identity or identity of this instance of the daemon?

  1. I propose to agree in terminology. I propose to call "the treasurer server" the system which actually sign transaction on behalf of service provider.

Now I can ask the main question. Who will be the treasurer server (who will sign the transactions): snet-cli or the instance of the daemon?

Because if it will be the daemon, then it kind of break all logic of snet-cli as owner of identity.

astroseger commented 5 years ago

I would argue that in the beta, it would be better to make snet-cli the treasurer server (only snet-cli will actually send transactions). It will allow as to solve the following issues in one place:

Because if we send transaction in snet-cli and in the daemon we need to solve these issue in two places.

vsbogd commented 5 years ago
  1. It will be code duplication: to write this server properly in snet-cli you will need to reproduce the code which is already implemented in daemon.
  2. Also supporting this infrastructure in daemon code base is simpler, because you have all things which plays with internal storage in one place.
  3. Anyway one need clear interface for claiming and daemon already has it, the only work which should be done is to provide snet-cli access to this API.

https://github.com/singnet/snet-cli/issues/111 and https://github.com/singnet/snet-cli/issues/67 can be solved in daemon code base as well.

vsbogd commented 5 years ago

Results of offline discussion with @astroseger:

astroseger commented 5 years ago

@raamb So the questions is: who will do it in the daemon?

raamb commented 5 years ago

@astroseger the Bangalore team will pick this up.

raamb commented 5 years ago

Discussed with @astroseger about this. We have a functional way of claiming funds for now, we will evaluate if this is absolutely required for Beta or not. As Sergey pointed out if Daemon is unable to support hardware wallets etc we will have to get this done. Initial estimates indicate that it would be 2-3 man days worth of work.

pennachin commented 5 years ago

This is required for the beta, of course. Unless I'm missing something, there is no reason the daemon should even know about hardware wallets, etc. The daemon is a server-side deployment component, it should not be the component signing these mainnet transactions. The cli is a "service owner admin" component, it makes far more sense for any signing of "real money" transactions to be done there.

One of the reasons for the Arbiter contract design was to prevent having to give the daemon private keys to a meaningfully funded wallet. That's still a requirement.

vsbogd commented 5 years ago

The way it is implemented right now means that daemon doesn't need access to the private key to collect and validate the payments. But to implement collecting payments as soon as possible we have implemented separate console tool which reuses daemon code to claim the payments. This tool can be used separately from daemon and its configuration. I agree that it makes sense to move payment management cli into snet-cli tool, but it requires additional development.

astroseger commented 5 years ago

@pennachin

In the current implementation "snet-daemon" has two completely separated components "the daemon" and "the treasurer server".

But in the current implementation "the treasurer server" is very "thin". It is actually a command line tool.

It makes a lot of sense to move "the treasurer server" functionality to snet-cli. It will make snet-cli only one owner of Ethereum identity. And, as I understand, it is very easy to do (see description from @vsbogd ).

And in the future I would suggest to build the real " treasurer server" around snet-cli, in order to have single Ethereum identity manager.

pennachin commented 5 years ago

@astroseger this makes a lot of sense to me.

vforvalerio87 commented 5 years ago

@astroseger agreed; much more elegant

raamb commented 5 years ago

I guess my comment "Daemon is unable to support hardware wallets etc" was the cause of some confusion, my apologies. I was referring to the treasurer component of the daemon by this statement not the Daemon itself.

astroseger commented 5 years ago

was closed with #162