Skip to content

Conversation

kare
Copy link
Contributor

@kare kare commented Nov 19, 2021

No description provided.

@linear
Copy link

linear bot commented Nov 19, 2021

ETH-184 Config package

The scripts should output config file, and that config file should go to NPM. This eliminates hand-copying addresses everywhere else.

This config package should live in network-contracts monorepo.

Config package structure should reflect multi-chain thinking: no more "main chain <> side chain" but just chains that are self-contained and maybe talk to each other over bridges or smth:

{
  "mainnet": { ... },
  "xdai": { ... },
  "dev_mainnet": { ... },
  "dev_xdai": { ... },
  "polygon": { ... },
  ...
}

Fundamentally the idea is to automate this google docs sheet: https://docs.google.com/document/d/1tygb8_9Y5AmzHzio3s3lM0rw_APGFT9hnNNutpUjO8c/edit#heading=h.6ynmi29odip

It will be the 3rd iteration on this config package idea, two previous iterations being:

So ideally it:

  • contains all "static configuration data" from production chains, manually updated
  • mixed with dev environment chains, automatically updated by the smart-contracts-init
    • hence it will be useful that smart-contracts-init and this config package live in the same monorepo, the network-contracts
  • is used by every streamr package everywhere that needs "static configuration data" (most of them), and no one needs to hand-copy things redundantly to different places again, also we can retire that google doc sheet.

Copy link
Contributor

@jtakalai jtakalai left a comment

Choose a reason for hiding this comment

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

Very nice! Let's get this into use. One very specific use case would be having streamr-client ingest a Network definition (or two, "mainnet" and "sidechain") and find everything it needs from there. That would already be a big win for maintainability.

@kare kare force-pushed the ETH-184-streamr-config-package branch from 80cdd04 to 12ba2e8 Compare December 16, 2021 15:05
@hpihkala
Copy link
Contributor

hpihkala commented Jan 24, 2022

@jtakalai @kare can we add to this:

  • Public/default RPC urls for each chain
  • The contracts recently deployed to Polygon
  • README changes describing how to use

And get it merged?

Copy link
Contributor

@jtakalai jtakalai left a comment

Choose a reason for hiding this comment

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

Good to get a first version out, so better merge it.

Things to think about though:

  • is the idea the we have a prod that mirrors dev? In that case the chain names should be precisely the same
  • or maybe we could just have them all side-by-side in the same structure, with an extra "testnet" attribute or something? This way they could all be "potentially available" in e.g. tests that for some reason want to read things from live network (or later in some kind of UI selector?). From technical point of view it wouldn't hurt really if we assume we're going to pick a chain by name anyway when we need one.

@jtakalai jtakalai requested a review from samt1803 February 3, 2022 14:07
"AMB": "0xaFA0dc5Ad21796C9106a36D68f69aAD69994BB64",
"DataUnionFactory": "0x4A4c4759eb3b7ABee079f832850cD3D0dC48D927",
"DataUnionTemplate": "0x36afc8c9283CC866b8EB6a61C6e6862a83cd6ee8",
"StorageNodeRegistry": "0xbAA81A0179015bE47Ad439566374F2Bae098686F",
Copy link
Contributor

Choose a reason for hiding this comment

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

streamRegistry and streamStorageRegistry could be added here

Copy link
Contributor

Choose a reason for hiding this comment

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

the storagenoderegistry is also wrong: current values for the docker dev env sidechain are:
streamRegistryChainAddress: '0x6cCdd5d866ea766f6DF5965aA98DeCCD629ff222',
nodeRegistryChainAddress: '0x231b810D98702782963472e1D60a25496999E75D',
streamStorageRegistryChainAddress: '0xd04af489677001444280366Dd0885B03dAaDe71D',

Copy link
Contributor

Choose a reason for hiding this comment

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

Please let's avoid using only "nodeRegistry" as any config variable. The NodeRegistry contract is instantiated for a few different purposes. Let's use names that describe the instance (not the class):

  • storageNodeRegistry
  • trackerRegistry

Copy link
Contributor

@samt1803 samt1803 left a comment

Choose a reason for hiding this comment

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

Looks good!

@samt1803 samt1803 self-requested a review February 3, 2022 14:25
Copy link
Contributor

@samt1803 samt1803 left a comment

Choose a reason for hiding this comment

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

Local docker dev env sidechain address is wrong and 2 other can be added there.
Rest looks good, verified all matic addresses.

"AMB": "0xaFA0dc5Ad21796C9106a36D68f69aAD69994BB64",
"DataUnionFactory": "0x4A4c4759eb3b7ABee079f832850cD3D0dC48D927",
"DataUnionTemplate": "0x36afc8c9283CC866b8EB6a61C6e6862a83cd6ee8",
"StorageNodeRegistry": "0xbAA81A0179015bE47Ad439566374F2Bae098686F",
Copy link
Contributor

Choose a reason for hiding this comment

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

the storagenoderegistry is also wrong: current values for the docker dev env sidechain are:
streamRegistryChainAddress: '0x6cCdd5d866ea766f6DF5965aA98DeCCD629ff222',
nodeRegistryChainAddress: '0x231b810D98702782963472e1D60a25496999E75D',
streamStorageRegistryChainAddress: '0xd04af489677001444280366Dd0885B03dAaDe71D',

@hpihkala
Copy link
Contributor

hpihkala commented Feb 3, 2022

  • is the idea the we have a prod that mirrors dev? In that case the chain names should be precisely the same

Nah I dont't think that needs to (or even should) be the goal.

  • or maybe we could just have them all side-by-side in the same structure, with an extra "testnet" attribute or something? This way they could all be "potentially available" in e.g. tests that for some reason want to read things from live network (or later in some kind of UI selector?). From technical point of view it wouldn't hurt really if we assume we're going to pick a chain by name anyway when we need one.

Exactly, they can very well be all defined in the same file. Apps can then depending on their environment say "use this, this and this chain", referring to the chains by name. So on app side config would then start to look something like this (example, not literal):

In local docker:

marketplaceChain=local-mainchain
dataUnionChain=local-sidechain
streamChain=local-sidechain

In prod:

marketplaceChain=ethereum
dataUnionChain=xdai
streamChain=polygon

Later on in a multi-chain setup we can give lists:

marketplaceChains=ethereum,xdai,polygon
dataUnionChains=xdai,polygon
streamChain=xdai,polygon

@kare kare requested a review from teogeb February 4, 2022 11:38
@kare kare force-pushed the ETH-184-streamr-config-package branch from a557027 to d71de56 Compare February 10, 2022 12:13
@kare kare force-pushed the ETH-184-streamr-config-package branch from ed3e2f0 to d6d3ffe Compare February 14, 2022 14:38
@kare kare force-pushed the ETH-184-streamr-config-package branch from d6d3ffe to 01738f3 Compare February 14, 2022 14:41
@kare kare force-pushed the ETH-184-streamr-config-package branch from 8edfc05 to e7b394f Compare February 14, 2022 16:23
@kare kare force-pushed the ETH-184-streamr-config-package branch from 965e39c to c41057c Compare February 14, 2022 19:10
@kare kare merged commit b35422f into master Feb 18, 2022
@kare kare deleted the ETH-184-streamr-config-package branch February 18, 2022 13:40
samt1803 added a commit that referenced this pull request Feb 23, 2022
* master-3: (44 commits)
  fix: fixed bug in subgraph if permission is set on a non-existing stream. It will now ignore the eth event
  release: @streamr/config 0.0.1
  build(config): version is not required by 'npm publish' (#170)
  feat: add @streamr/config package (#84)
  comment: updated deployed-on comment in StreamregistryV3
  add V3 streamregistry to preloaded parity chain; fix buildscript some more
  fixed docker-dev-chain-init script
  ETH-244: simplify network contracts workspace (#153)
  replaced an outdated signing library with its new version, replaces a require with import, changed a few types around to make it work (#151)
  Eth 228 streamregistry contract upgrade (#150)
  build: update dependencies (#147)
  chore: update .gitignore
  build(deps-dev): bump eslint-config-streamr-nodejs (#124)
  build(deps-dev): bump @typescript-eslint/eslint-plugin (#142)
  build(deps-dev): bump @types/mocha in /packages/smartcontracts (#125)
  build(deps): bump dotenv in /packages/chainlink-ens-external-adapter (#141)
  fix: fix prod deployment script; Add README part on how to do the prod deployment.
  fix: small fix in theGraph: when through trusted account implcitly creating a stream by setting its metadata the createdAt timestamp was not set. Now it will be
  feat: added created-at and updated-at timestamps to stream in subgraph (#139)
  Update storage node metadata #140
  ...
samt1803 added a commit that referenced this pull request Feb 28, 2022
* master-3:
  fix: fixed bug in subgraph if permission is set on a non-existing stream. It will now ignore the eth event
  release: @streamr/config 0.0.1
  build(config): version is not required by 'npm publish' (#170)
  feat: add @streamr/config package (#84)
  comment: updated deployed-on comment in StreamregistryV3
  add V3 streamregistry to preloaded parity chain; fix buildscript some more
  fixed docker-dev-chain-init script

# Conflicts:
#	package-lock.json
#	package.json
samt1803 added a commit that referenced this pull request Feb 28, 2022
* master-3:
  fix: fixed bug in subgraph if permission is set on a non-existing stream. It will now ignore the eth event
  release: @streamr/config 0.0.1
  build(config): version is not required by 'npm publish' (#170)
  feat: add @streamr/config package (#84)
  comment: updated deployed-on comment in StreamregistryV3
  add V3 streamregistry to preloaded parity chain; fix buildscript some more
  fixed docker-dev-chain-init script

# Conflicts:
#	package-lock.json
#	package.json
samt1803 added a commit that referenced this pull request Mar 9, 2022
* master-3: (21 commits)
  feat(config): load config by detecting environment from NODE_ENV (#171)
  feat(config): allow multiple RPC addresses (#201)
  fix: fixed regression bug in migration script
  ci: bump actions/checkout from 2.4.0 to 3 (#192)
  ci: bump actions/stale from 4.1.0 to 5 (#191)
  Eth 138 migrate streams to ocr (#190)
  ci: bump actions/setup-node from 2.5.1 to 3 (#189)
  ci: remove root Npm package from Dependabot config
  ci: increase Dependabot PR limit for smartcontracts
  docs(config): Use correct nomenclature in README
  test(config): rename test
  build: update package.json files based on versions in package-lock.json (#176)
  build(deps-dev): bump solidity-coverage from 0.7.17 to 0.7.20 (#174)
  build(deps-dev): bump @types/node from 16.11.7 to 16.11.25 (#172)
  fix: fixed bug in subgraph if permission is set on a non-existing stream. It will now ignore the eth event
  release: @streamr/config 0.0.1
  build(config): version is not required by 'npm publish' (#170)
  feat: add @streamr/config package (#84)
  comment: updated deployed-on comment in StreamregistryV3
  add V3 streamregistry to preloaded parity chain; fix buildscript some more
  ...

# Conflicts:
#	.gitignore
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.

4 participants