-
Notifications
You must be signed in to change notification settings - Fork 36
chore(sponsored-feeds): use JSON format for the sponsored feed data for EVM #746
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
fdfcd9c
to
b731a07
Compare
b731a07
to
bc00d71
Compare
const networkImports = { | ||
ethereum_mainnet: () => | ||
import( | ||
"../pages/price-feeds/sponsored-feeds/data/evm/ethereum_mainnet.yaml" | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping separate yaml files for each network, so it is easy to access/modify the data.
@@ -55,4 +55,4 @@ The addresses represent the price feed account for shard 0 of the relevant price | |||
| FARTCOIN/USD | `2t8eUbYKjidMs3uSeYM9jXM9uudYZwGkSeTB4TKjmvnC` | `58cd29ef0e714c5affc44f269b2c1899a52da4169d7acc147b9da692e6953608` | | |||
| ACRED/USD | `6gyQ2TKvvV1JB5oWDobndv6BLRWcJzeBNk9PLQ5uPQms` | `40ac3329933a6b5b65cf31496018c5764ac0567316146f7d0de00095886b480d` | | |||
| WTI1M/USD | `nt1JuGVRBJNrvMpdZt9RJnxVSxRuoeVx5LRq3R1bS16` | `eca3fb7e6be5be55a01678ee00f6405b9e4986e32d539ccd2a06a57e0c615880` | | |||
| PUMP/USD | `HMm3GPbdnqGwbkTnUUqCFsH8AMHDdEC3Lg8gcPD3HJSH` | `7a01fca212788bba7c5bf8c9efd576a8a722f070d2c17596ff7bb609b8d5c3b9` | | |||
| PUMP/USD | `HMm3GPbdnqGwbkTnUUqCFsH8AMHDdEC3Lg8gcPD3HJSH` | `7a01fca212788bba7c5bf8c9efd576a8a722f070d2c17596ff7bb609b8d5c3b9` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is unrelated to this PR, it is from precommit prettier for merged changes.
priceDeviation: number; | ||
} | ||
// Import the data for each network. The data is in the form of a yaml file. | ||
const networkImports = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than importing all networks here, can't you make a component which will take the yaml on run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing import of the json files in the mdx file directly, not using network key anymore.
import { useState } from "react"; | ||
import copy from "copy-to-clipboard"; | ||
|
||
export const useCopyToClipboard = (timeout: number = 2000) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we abstract this method and use it with copyAddress?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the idea is to use it in other places instead of copy()
, that is kept it in utils folder.
Will replace it across as part of another PR that would be specific to this.
As discussed, yaml is heavyweight and could impact the performance. We will use json files as it is light weight and updating would be easy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestions but the big stuff LGTM!
@@ -22,56 +20,82 @@ interface SponsoredFeedsTableProps { | |||
/** | |||
* Helper functions | |||
*/ | |||
// Convert time_difference (seconds) to human readable format | |||
const formatTimeUnit = (seconds: number): { value: number; unit: string } => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DurationFormat for this. It's a bit overkill for our needs as we don't plan to localize pretty much ever, certainly not any time soon, but in general it's probably wise to stick with standards whenever possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, using Intl.DurationFormat seems like a overkill, the code earlier was easy to read imo. Though I agree with your point of sticking around with standards. So added Intl.DurationFormat in formatTimeUnit
in commit 00fb7c1
Was using regex with Intl.DurationFormat
, so instead using https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DurationFormat/formatToParts as this returns an array with unit and value. Check the latest commit 9647761
</span> | ||
</div> | ||
); | ||
const renderUpdateParams = (feed: SponsoredFeed, isDefault: boolean) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a component since it returns jsx, and the only change would be renaming it from renderUpdateParams
to UpdateParams
, and changing the arguments to an object.
By using a component, React can optimize rerenders and the component tree. By using a function, react is not aware of this subtree as a component and applies no render optimizations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining the rationale.
Addressed in commit 49728da
Description
Use the JSON file in the same structure as defined in the deployments repository so it is easy to update when a price feed data changes.
Type of Change
Areas Affected
There is no change in the UI. It remains the same as before.

Checklist
pre-commit run --all-files
to check for linting errorsRelated Issues
Closes #
Additional Notes
Contributor Information
Screenshots