Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

TrackerRegistry #40

Merged
merged 10 commits into from Sep 23, 2020
Merged

TrackerRegistry #40

merged 10 commits into from Sep 23, 2020

Conversation

mirpo
Copy link
Contributor

@mirpo mirpo commented Sep 22, 2020

  • Added TrackerRegistry
  • Added into CI integration tests (using streamr-docker-dev)
  • Updated all libs
  • Added idea files and new npm scripts

src/index.js Outdated Show resolved Hide resolved
src/utils/fetchTrackers.js Outdated Show resolved Hide resolved
@mirpo mirpo requested a review from harbu September 22, 2020 13:31
@mirpo mirpo changed the title fetchTrackers and hashring TrackerRegistry Sep 22, 2020
@mirpo mirpo requested a review from juhah September 22, 2020 16:14
contractAddress, trackerRegistryConfig, jsonRpcProvider, servers, algorithm = 'sha256', hashRingOptions
}) => {
const trackerRegistry = new TrackerRegistry({
contractAddress, trackerRegistryConfig, jsonRpcProvider, servers, algorithm, hashRingOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the trackerRegistryConfig really necessary as a param? If it lives in this repo it could just use it by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the only difference between dev, staging and prod is contractAddress - yes, sure. But who can clarify this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, I don't know either... just thinking about the usage of the helper that it's an extra step if it can already assume that it's the contract it will be using. But ok for me to go with this as well.

@mirpo mirpo requested a review from harbu September 23, 2020 07:44
@mirpo mirpo merged commit bdb4881 into master Sep 23, 2020
@mirpo mirpo deleted the trackers branch September 23, 2020 07:47
Copy link
Contributor

@timoxley timoxley left a comment

Choose a reason for hiding this comment

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

late review, lgtm, left a PSA about jest async + done.

])
})

test('throw exception if address is wrong (ENS)', async (done) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI async (done) is going away when jest-circus lands:
jestjs/jest#9129

Scheduled for Jest 27:
jestjs/jest#6295

I opened a related issue here:
jestjs/jest#10529

Copy link
Contributor

Choose a reason for hiding this comment

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

To get rid of the done the equivalent would probably be this:

await expect(await getTrackerRegistry({
  contractAddress: 'address', jsonRpcProvider
})).rejects.toThrow('Error: network does not support ENS')

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants