Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add domain for user profile #62

Merged
merged 25 commits into from
Nov 21, 2024

Conversation

HenrikJannsen
Copy link
Contributor

No description provided.

@HenrikJannsen HenrikJannsen marked this pull request as draft November 17, 2024 17:01
@HenrikJannsen HenrikJannsen marked this pull request as ready for review November 18, 2024 10:39
@HenrikJannsen HenrikJannsen force-pushed the add-user-profile-domain branch from d5e3f73 to 5b39d49 Compare November 18, 2024 10:42
Copy link
Collaborator

@rodvar rodvar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work @HenrikJannsen

Runtime

  • I've tested in android pixel 6 (crashes, but expected from your comments) and in emulator. I'll test in all 3 apps on once this first pass of review gets done
  • can get the emulator to run and communicate with running local bisq2-rest-api
  • can see the profile creation working in the logs and the screen
  • however, I can't go past the screen when clicking next nothing happens and no error is logged
  • some other errors seen:
kotlinx.serialization.json.internal.JsonDecodingException: Unexpected JSON token at offset 2: Encountered an unknown key 'error' at path: $`
                                                                                                   Use 'ignoreUnknownKeys = true' in 'Json {}' builder to ignore unknown keys.
                                                                                                   JSON input: {"error":"Could not find a selected user identity"}

Code

I don't wanna be too verbose here, I think when you read my individual comments you'll find the resolution to our discussions. Please reach out if something is not clear.

Regarding the Ktor addition. I was working on this as well but in a separate new networking module in an effort to avoid making the androidModule grasp those dependencies when its not gonna be used.

I think I'll drop that for now until we get this one to be merged and then work on that refactor later on.

I contribute with you by finishing the lifecycle setup so you can reuse in your PR as well, might also include the solution for the Dispatchers.IO for you
cheers!

@rodvar
Copy link
Collaborator

rodvar commented Nov 19, 2024

@HenrikJannsen done this for you

you'll probably get conflicts with the CreateProfile screen related code on rebase. I humbly suggest that we keep the presenter view defined interface (you'll see why in the code) but it's up to you for this particular case. cheers!

@HenrikJannsen
Copy link
Contributor Author

  • I can't go past the screen when clicking next nothing happens and no error is logged

I commented out the code to go to next screen in the presenter for dev testing. see comments there. i wanted to test several use cases and as the UI is not yet prepared for that i did it with changing the presenter. I will add a commit to undo that. But would be good as next step to make the UI behaving to the intended use case.

  1. After loading screen -> check if there is an existing user profile.
    1a. If there is an existing user profile, do not show create user profile screen, but show user profile is some not yet defined way (right top corner in Desktop shows user profile).
    1b. If there is no existing user profile, show create profile screen.

Additionally: Show busy animation when generating keypair and when creating the profile. The domain model contains 2 boolean inProgress properties for that.

@HenrikJannsen
Copy link
Contributor Author

Regarding the Ktor addition. I was working on this as well but in a separate new networking module in an effort to avoid making the androidModule grasp those dependencies when its not gonna be used.

Ah that is good. But maybe we call it 'client' instead of networking, as networking happens also in the node when all is delegated to the libraries.
Is it basically the code from shared.domain.client.
We should also move the Bisq2 replicated models (annotated with @serializable) which are only used for client over there.

Btw. I did not clean up the gradle dependencies, there might be too many and they should be moved to toml file.... also there are those issues with the exclusion of the logging lib... But I guess you have better ideas when it comes to gradle stuff..

@HenrikJannsen HenrikJannsen force-pushed the add-user-profile-domain branch from 32c95f7 to 51a63f1 Compare November 19, 2024 04:08
@HenrikJannsen HenrikJannsen force-pushed the add-user-profile-domain branch from 5778656 to 066108b Compare November 20, 2024 13:53
@rodvar rodvar force-pushed the add-user-profile-domain branch from 49b7d73 to 5daa232 Compare November 20, 2024 22:17
Copy link
Collaborator

@rodvar rodvar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HenrikJannsen awesome work, this looks so much better.

I'll update the architecture doc to show that presenters can depend on services as well for domain interaction.

I've taken the liberty of doing a small contribution to address the small important stuff that was missing from the my previous review - please have a look at it to see if we agree on it.

Finally, I added some nits, most of it optional or to be done in a subsequent PR. I would say the most important one is the one referred to the HttpClient (Ktor) as using it as is adds boilerplate that can be avoided if configured properly (making it serialize/deserialize automatically)

Let me know your thoughts, I can see this being merge by the end of today :D !

Remove demo code from MainNodePresenter and inti code to AndroidApplicationService.
Add generateKeyPairInProgress and createAndPublishInProgress properties.
Add createSimulatedDelay method to ClientUserProfileServiceFacade

Supplier is then used in DI and provides the services from the AndroidApplicationService when used by a lazy getter.
This hack is needed as AndroidApplicationService requires some constructor argumewnts which are not present as application startup but only after the mainActiviy is created.
We startup KOIN in the MainApplication.onCreate method.
We use the MainNodePresenter which gets called its onViewAttached once the mainActiviy is ready and then we can set the applicationService inside the supplier.

I don't like that unsafe lateinit setter strategy and have proposed an alterantive solution in my POC which would work with constructor injection. Maybe there are some hacks in KOIN to get the done in a different manner, but the main problem is that KOIN gets started at the MainApplication.onCreate and not on the MainActivity.onCreate. Only at that moment we have all the basic dependencies from the Android UI framework.
But even if we would move that to the MainActivity.onCreate, we would need to pass the properties to the module in some way.
Add DI config to AndroidClientModule
Remove ICreateProfilePresenter as there is only one instance and not expected a use case for multiple instances.
Add UserProfileRepository holding model and serviceFacade
Adjust Screen and Presenter.

Discussion:
To me the usage of the repository does not make much sense here.
The 2 relevant methods do not map into the CRUD pattern which seems to be the design philosophy for repositories.
I would suggest to use it only when teh CRUD pattern and persistence is used and appropriate. And to inject the model and the service facade to the presenter in the other cases.
Otherwise we just create a useless wrapper.
@rodvar: If you would implement the usage here in a different way, can you provide with the given use case an implementation example?
On iOS simulator we need to use localhost, not 10.0.2.2
Use Dispatchers.Main instead of Dispatchers.IO as it seems iOS does not support that.
…ityIds methods.

In the CreateProfilePresenter at onLaunched there are commented out some different dev test modes;
- Use hasUserProfile to check if create user profile screen should be shown
- onGenerateKeyPair if no profile is present
- applySelectedUserProfile if one is present and we apply the data to the screen. This is only for dev testing. We have to display the selected user profile in the app in a TBD way.

Tested with all 3 apps and working.
Requires latest Bisq 2 main version and a running rest api.
Go to next screen on onCreateAndPublishNewUserProfile completed.
Remove unneeded coroutineScope.launch blocks
HenrikJannsen and others added 5 commits November 21, 2024 13:38
This only provides the domain part. It is not yet connected to the UI as its still unclear to me what patter to use for that.

When using the Client version one need to instantiate th ClientApplicationBootstrapFacade and call initialize(). Could be done in the presenter...
 - fix double call on view attached for the special case of the
   MainPresenter
 - remove double static configuration of bisq core jars (left in
   MainApplication override only)
 - provide single instance of android memory report service and adjust
   usages
 - cleanup/comment dead code
 - cleanup unused imports
@HenrikJannsen HenrikJannsen force-pushed the add-user-profile-domain branch from 2846f9c to 9f424b1 Compare November 21, 2024 06:38
import io.ktor.client.request.setBody
import io.ktor.http.contentType

class ApiRequestService(val httpClient: HttpClient, host: String) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Copy link
Collaborator

@rodvar rodvar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@rodvar rodvar merged commit 83cc9bc into bisq-network:main Nov 21, 2024
1 check passed
@HenrikJannsen HenrikJannsen deleted the add-user-profile-domain branch November 22, 2024 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants