search

datacraft-dsc / starfish-java

Developer Toolkit for the Data Ecosystem
3 stars 2 forks source link

Implement Resolver functionality #37

Closed mikera closed 4 years ago

mikera commented 5 years ago

It should be possible to get a DDO (as a String) for any given DID registered on-chain.

It should also be possible to register a DDO for a DID given an ethereum account

mikera commented 4 years ago

Moved back to in-progress, until we have a stable API proposal that must include:

Needs to be will explained in JavaDocs and doc site

mikera commented 4 years ago

@shark8me adding you since you are a consumer of this

ilyabukalov commented 4 years ago

Done

mikera commented 4 years ago

@ilyabukalov Where is this documented currently? If I want to use the resolver in a "Hello World" type case how do I do it?

mikera commented 4 years ago

@pedrorgirardi FYI this is the functionality you will need in surfer, great if you could review.

ilyabukalov commented 4 years ago

Attaching the reference https://github.com/DEX-Company/starfish-java/blob/9613cf263a53e374a61cd175223cde09b95b0cf9/src/integration-test/java/keeper/DexResolverTest.java#L78

mikera commented 4 years ago

Thanks Ilya.

Some comments:

mikera commented 4 years ago

@ayush the construction of the credential map looks a bit unwieldy from a user perspective can you simplify this? I'm expecting something like RemoteAccount.create(username, password). We shouldn't be forcing users to construct hashmaps just to do this!

pedrorgirardi commented 4 years ago

@mikera my use case was a function that gets an Agent from a DID so then we can use the get-asset function and pass the resolved Agent as a parameter.

starfish-clj currently uses a LocalResolverImpl to get a DDO for the specific DID and then create a remote Agent. The problem then is that the resolved Agent might require authentication and we don't have a solution for that - independently of the Resolver implementation - e.g., LocalResolverImpl, DexResolver.

ilyabukalov commented 4 years ago
  • Most important from a user perspective is a function that gets an agent or asset from a given DID. It should not be tied to a specific implementation (DEXResolver, RemoteAgent) since it needs to work with other implementations e.g. Squid/Ocean assets. Where is the public API for this? Perhaps @pedrorgirardi you can comment further on what you need here?

Starfish as library does not have configs by design and our decision. The config which is used here is created specially for integration tests. Current "real users" of DexResolver are only admiral and surfer. Both of them have required configuration. And no one else except them have this configuration @pedrorgirardi Could you clarify and confirm whether I am correct and do you need anything else more than defined interface (except authorization as you described above)?

  • Line 6-7: Should wrap these exception types with appropriate starfish exceptions (avoid exposing web3-specific implementation details)

ok. Can be done.

  • Line 36-37: Why two different configs? Please document. Also must be possible to use starfish as a library with minimal/zero config so what are the defaults?

It is just for demonstration in integration tests. The case where you register from one resolver (assuming it is separated computer) and resolving from another computer. And as well for the specific case with permission control, where write/update DDO is not allowed by another account. These two configs are exactly the same except that they have different account. This behaviour with permission control I described here in "Resolver" section. There is no defaults because of there is no config as I said above. Universal Resolver as part of starfish is impossible since there is no configs on starfish (where resolver should connect) or we need to review and change again the approach to configs in starfish-java. @AyushVerma2 could you help and clarify this? Since there are still a lot of questions I will create separated document in DEP with step by step explanation with coding.

It is exactly what I have done. It works but I didn't commit it because it has another issue which I described here

ilyabukalov commented 4 years ago

Authorization is not clear for everyone now. Probably @shark8me is the first person who has developed some first approach. We need to listen him at first to continue our further steps. For a while, regarding concrete case with DexResolver integration, we decided on our latest call to use hardcoded values of authorization.

ilyabukalov commented 4 years ago

@mikera my use case was a function that gets an Agent from a DID so then we can use the get-asset function and pass the resolved Agent as a parameter.

Exactly. This would perfectly suit to existing starfish interface.

mikera commented 4 years ago

Starfish as library does not have configs by design and our decision. The config which is used here is created specially for integration tests.

So what do we expect a library user to do? What is idiomatic usage of the resolver in user code?

ilyabukalov commented 4 years ago

My opinion is based on the limited circumstances which we have now by the design:

  1. Starfish java does not provide any config by design (if it is wrong design? Then lets rebuild it @AyushVerma2).
  2. DexResolver must have an account on chain, it must be initialized somehow by an account. => which cause
  3. Being part of starfish DexResolver is supposed to accept some external config created by user. There is no other choices in the current design of starfish. The config format is very simple and I will describe it in new DEP document as I commented above.

Moreover, there are cases where there are several networks with several different accounts or one network with several different accounts. So thats why config (which contains both network+account settings) should be separated of code to provide such customization for user.

AyushVerma2 commented 4 years ago

@ayush the construction of the credential map looks a bit unwieldy from a user perspective can you simplify this? I'm expecting something like RemoteAccount.create(username, password). We shouldn't be forcing users to construct hashmaps just to do this!

I will be fixing this by this week

AyushVerma2 commented 4 years ago

My opinion is based on the limited circumstances which we have now by the design:

  1. Starfish java does not provide any config by design (if it is wrong design? Then lets rebuild it @AyushVerma2).
  2. DexResolver must have an account on chain, it must be initialized somehow by an account. => which cause
  3. Being part of starfish DexResolver is supposed to accept some external config created by user. There is no other choices in the current design of starfish. The config format is very simple and I will describe it in new DEP document as I commented above.

Moreover, there are cases where there are several networks with several different accounts or one network with several different accounts. So thats why config (which contains both network+account settings) should be separated of code to provide such customization for user.

Currently Starfish lib used config only for running IT test case.In Src code there should be no reference of it. @ilyabukalov : let discuss on this tomorrow.

AyushVerma2 commented 4 years ago

@ayush the construction of the credential map looks a bit unwieldy from a user perspective can you simplify this? I'm expecting something like RemoteAccount.create(username, password). We shouldn't be forcing users to construct hashmaps just to do this!

I will be fixing this by this week

This is fixed and code pushed to develop branch.Please refer testcase below to verify: In TestRemoteAgentTest_26.java, testCreatRemoteAccount_WithUsernamePassword testCreatRemoteAccount_WithWrongUsernamePassword

ilyabukalov commented 4 years ago

Thanks it works now https://github.com/DEX-Company/starfish-java/commit/83c7720b5d43af82e465844d94df6efbbf43aadc

ilyabukalov commented 4 years ago
  • Line 6-7: Should wrap these exception types with appropriate starfish exceptions (avoid exposing web3-specific implementation details)

https://github.com/DEX-Company/starfish-java/commit/8994738f3dcd2a577707f2c3eb715b905fff2e9b Transaction exception is removed. CipherException has dependency from squid. But it is rather formal dependency than real functioning. I will try to do refactoring to remove squid at all (and check whether it is possible)

ilyabukalov commented 4 years ago

https://github.com/DEX-Company/starfish-java/commit/0d55477447e823484642e4db60f1b428808114f7

ilyabukalov commented 4 years ago

DEP created

ilyabukalov commented 4 years ago

Please review everything and close.

AyushVerma2 commented 4 years ago

We need Integration Testing as a part of Starfish-java.