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

#chat/registry #262

Merged
merged 7 commits into from
Jun 13, 2022
Merged

#chat/registry #262

merged 7 commits into from
Jun 13, 2022

Conversation

llbartekll
Copy link
Contributor

  • Add registry manager
  • update client api with specs
  • add Concurrency

import WalletConnectUtils
import WalletConnectKMS

actor RegistryManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

What the difference between Manager and Engine naming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am always scratching my head when giving them a names. in this context it just sounds better to me. Any suggestions?

}

override func tearDown() {
registry = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

May be not necessary to set nil at tearDown ? We setting new value in setUp

}

class KeyValueRegistry: Registry {
actor KeyValueRegistry: Registry {
Copy link
Contributor

Choose a reason for hiding this comment

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

What benefits actor gives us here?

We could make it class and make registryStore private. So it couldn't be mutated from outside too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true it is better registryStore is private
but I think making it an actor gives thread safety

So it couldn't be mutated from outside too

It could be mutated by calling register() from different threads
anyway this class is mimicking an http client right now

Copy link
Contributor

@flypaper0 flypaper0 left a comment

Choose a reason for hiding this comment

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

async usage looks nice and convenient to use

Base automatically changed from develop to main June 9, 2022 12:20
@sekimondre
Copy link

This PR is pointing to main after the develop merge, need to fix it

@llbartekll llbartekll changed the base branch from main to develop June 10, 2022 06:26
@llbartekll llbartekll merged commit b0af971 into develop Jun 13, 2022
@llbartekll llbartekll deleted the #chat/registry branch June 13, 2022 06:24
@flypaper0 flypaper0 mentioned this pull request Jul 5, 2022
llbartekll added a commit that referenced this pull request Jul 5, 2022
* Add registry manager

* Update api methods

* update resolve

* Add description to api methods

* update invite method

* remove teardown

* make registryStore private
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants