-
Notifications
You must be signed in to change notification settings - Fork 14
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
Dashboard: stats widget #229
Conversation
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/aragon/court-dashboard/lne7j19dz |
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.
Couple of quick clean up comments, and then we'll likely want to re-work the hooks for fetching the stats.
Let's test this a bit more on mainnet and the rinkeby environments, so it works properly there!
src/components/Activity/assets/activity-icon-execute-ruling.svg
Outdated
Show resolved
Hide resolved
src/utils/known-tokens.js
Outdated
decimals: 18, | ||
symbol: 'ANJ', | ||
}, | ||
rpc: { |
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.
Do we use rpc
for rinkeby too, or should we provide a rinkeby config here?
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.
oh that was i mistake to test it locally , but maybe we should only leave here the mainnet config since we are not going to fetch the price for another environments
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.
That sounds reasonable too. Are these configs only used for fetching price (would be nice to comment this or make that assumption a bit more obvious)?
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 is only for fetching the price!, cool will comment something
const ethEndpoint = networkConfigs[getNetworkType()].nodes.defaultEth | ||
|
||
const ethProvider = useMemo( | ||
() => (ethEndpoint ? new Providers.JsonRpcProvider(ethEndpoint) : null), |
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.
A note for later: I think for now this is totally fine, since this provider uses http (it's maybe just a little memory in-efficient), but when we switch to websockets, we'll most likely want to initialize a singleton provider to maintain a single websocket connection for all contracts.
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.
ok, let's keep that in mind
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 is looking good for me 🏁🏁🏁
src/providers/CourtConfig.js
Outdated
// import environment from '../environment' | ||
import { useCourtConfigSubscription } from '../hooks/subscription-hooks' | ||
import { getNetworkType } from '../lib/web3-utils' | ||
import { networks } from '../networks' | ||
// import { getNetworkType } from '../lib/web3-utils' | ||
import { getNetwork } from '../networks' | ||
|
||
const CHAIN_ID = environment('CHAIN_ID') | ||
// const CHAIN_ID = environment('CHAIN_ID') |
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.
Let's remove this comments
src/hooks/useCourtContracts.js
Outdated
@@ -578,7 +582,7 @@ export function useTotalANTStakedPolling(timeout = 1000) { | |||
let cancelled = false | |||
let timeoutId | |||
|
|||
if (getNetworkType() === 'local') { | |||
if (getInternalNetworkName() === 'local') { |
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.
Should we use isLocalOrUnknownNetwork
?
src/endpoints.js
Outdated
@@ -27,7 +27,7 @@ function getAPIBase() { | |||
export default function endpoints() { | |||
const [API_BASE_HTTP, API_BASE_WS] = getAPIBase() | |||
const networkType = | |||
getNetworkType(CHAIN_ID) === 'local' ? 'rpc' : getNetworkType(CHAIN_ID) | |||
getNetworkType(CHAIN_ID) === 'private' ? 'rpc' : getNetworkType(CHAIN_ID) |
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.
Same here ? isLocalOrUnknownNetwork
Co-Authored-By: Brett Sun <[email protected]>
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.
🙌 🙌 🙌
No description provided.