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

feat(api,ui): use feedFullName as id for data-feeds #66

Merged
merged 0 commits into from
Aug 31, 2021

Conversation

Tommytrg
Copy link
Member

@Tommytrg Tommytrg commented Aug 25, 2021

Update feed and result-requests collections adding a new field (feedFullName) to avoid use random generated id.
To achieve this, the following changes have been made to feed and result-requests:

feed

  • feedFullName field has been added
  • request field has been removed

result-requests

  • feedFullName field has been added
  • feedId field has been removed
  • label field has been removed
  • address field has been removed

Models before the migration are:

Feed {
  id: String! 
  name: String!
  address: String!
  lastResult: String
  label: String!
  network: String!
  requests: [ResultRequest]!
  color: String!
  blockExplorer: String!
}
ResultRequest {
  id: String!
  feedId: String!
  result: String!
  label: String!
  requestId: String!
  address: String!
  timestamp: String!
  drTxHash: String!
  error: String
}

Models after the migration were:

Feed {
  id: String!
  address: String!
  blockExplorer: String! 
  color: String!
  feedFullName: String!
  label: String!
  name: String!
  network: String!
}
ResultRequest {
  id: String!
  drTxHash: String!
  error: String
  feedFullName: String!
  requestId: String!
  result: String!
  timestamp: String!
}

In addtion, this PR renames network names to discern between different chains and their networks
Close #62

@aesedepece
Copy link
Member

I have the hunch that we may want to add the number of decimal digits in the fullName, e.g. ethereum-mainnet_btc-usd_3, as I can foresee that it could happen that we publish some feed with 3 digits, then people start using, and later someone needs 6 digits, but we can't simply replace the former because it would be a breaking change.

packages/api/src/repository/Feed.ts Outdated Show resolved Hide resolved
packages/api/src/server.ts Outdated Show resolved Hide resolved
packages/api/test/web3Middleware/dataFeeds.ts Outdated Show resolved Hide resolved
@Tommytrg
Copy link
Member Author

Tommytrg commented Aug 26, 2021

I have the hunch that we may want to add the number of decimal digits in the fullName, e.g. ethereum-mainnet_btc-usd_3, as I can foresee that it could happen that we publish some feed with 3 digits, then people start using, and later someone needs 6 digits, but we can't simply replace the former because it would be a breaking change.

Then, the data-feed-explorer would show two different feeds for the same one. From now on, we have this information in the contract so we can store it inresult-request its digits to avoid creating a new feed if the digits change. What do you think?

@aesedepece
Copy link
Member

I have the hunch that we may want to add the number of decimal digits in the fullName, e.g. ethereum-mainnet_btc-usd_3, as I can foresee that it could happen that we publish some feed with 3 digits, then people start using, and later someone needs 6 digits, but we can't simply replace the former because it would be a breaking change.

Then, the data-feed-explorer would show two different feeds for the same one. From now on, we have this information in the contract so we can store it inresult-request its digits to avoid creating a new feed if the digits change. What do you think?

I mean, in that case we would definitely want to list 2 different feeds on the site.

@Tommytrg Tommytrg force-pushed the refactor-id branch 6 times, most recently from bd09e30 to aa95f43 Compare August 27, 2021 08:39
@Tommytrg Tommytrg force-pushed the refactor-id branch 2 times, most recently from 36a2639 to 209fa0e Compare August 27, 2021 08:58
@Tommytrg Tommytrg force-pushed the refactor-id branch 10 times, most recently from a226592 to c6c1baa Compare August 30, 2021 16:18
Comment on lines 54 to 59
ETHEREUM_MAINNET_PROVIDER: $ETHEREUM_MAINNET_PROVIDER
ETHEREUM_GOERLI_PROVIDER: $ETHEREUM_GOERLI_PROVIDER
ETHEREUM_KOVAN_PROVIDER: $ETHEREUM_KOVAN_PROVIDER
ETHEREUM_RINKEBY_PROVIDER: $ETHEREUM_RINKEBY_PROVIDER
CONFLUX_TESTNET_PROVIDER: $CONFLUX_TESTNET_PROVIDER
CONFLUX_MAINNET_PROVIDER: $CONFLUX_MAINNET_PROVIDER
Copy link
Member

Choose a reason for hiding this comment

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

We can't afford having to specify providers here. This should proably be on each feed's configuration or whatever, as in an ideal world, we should be able to add feeds without doing PRs, simply by modifying config files.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are adding it in this way because we are using providers with secrets instead of the Portainer's gateway. In other PR, we can avoid modifying this file and the provider's dictionary every time we want to support a new network.

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.

Allow changing address for a price feed (e.g. use deterministic IDs)
3 participants