This repository was archived by the owner on Sep 4, 2024. It is now read-only.
Conversation
jim-toth
suggested changes
Sep 18, 2023
| @@ -1,3 +1,5 @@ | |||
| @import 'https://api.mapbox.com/mapbox-gl-js/v2.6.1/mapbox-gl.css'; | |||
Contributor
There was a problem hiding this comment.
- This introduces a new, centralized, third-party dependency. Instead, you could just copy the contents of this css file and add it as its own file under
assets/styles. Of course, you will have to ensure the license of the css file allows such a thing. - This css file version mis-matches the installed package (
v2.15.0).
| "ardb": "^1.1.10", | ||
| "arweave": "^1.13.7", | ||
| "ethers": "^6.1.0", | ||
| "ip-geolocation-api-javascript-sdk": "^1.0.9", |
Contributor
There was a problem hiding this comment.
- On the surface this seems like a very sketchy package to install seeing as it has ~100 downloads per week on npm. It's very small and after further inspection it's just an http wrapper around the Geolocation API Service...
- ...which you're not even using 😄 so you should remove this dependency entirely.
|
|
||
| const getLatLonFromIPs = async (ips: string[]): Promise<any[]> => { | ||
| try { | ||
| const response = await $fetch('https://api.ipgeolocation.io/ipgeo-bulk?apiKey=cc09f9516dea4a9e804f6885921f231f', { |
Contributor
There was a problem hiding this comment.
- This introduces a new, centralized, third-party data source where previously the dashboard does not have any. All data sources for the dashboard are decentralized from blockchain data providers (arweave gateways, warp DRE nodes, eth jsonrpc, etc.). If this service were to disappear overnight, this feature of the dashboard would break.
- This also may not scale if hundreds of users are requesting simultaneously. This also exposes user IP addresses to this, imo, sketchy IP resolution service.
- The work to resolve lat/long from ip should not be taking place on the client. Instead, this could be regularly calculated by the
validatorand published to Arweave as another metrics entity type. - IP resolution isn't that difficult and shouldn't necessarily rely on a third party service of questionable reputation.
|
|
||
| try { | ||
| const responseData: any = await $fetch( | ||
| "https://onionoo.torproject.org/details" |
Contributor
There was a problem hiding this comment.
- Similar to the comment above, this introduces a new, centralized, third-party dependency directly to the dashboard. Yes, technically the
validatorcurrently looks at Onionoo, but this may not always be the case. This also may not scale if hundreds of users are requesting simultaneously. This also exposes user IP addresses to Onionoo. - Again, similar to the comment above, this could be part of the
validatorand published as a new metrics entity type.
| useDistribution, | ||
| useFacilitator, | ||
| useRelayRegistry | ||
| } from '~/composables' |
Contributor
There was a problem hiding this comment.
Technically a correct improvement but please do try to keep unrelated changes separate 😄
Contributor
There was a problem hiding this comment.
I'm wondering if we even need Mapbox or if there is some other open source map project that doesn't rely on a third party API and access tokens. I'm not really sure the point of the API or what it's actually doing other than gating what could be entirely client-side features.
Essentially, given a list of lat/long coords, put them on a map. The earth doesn't change much 😄
Contributor
There was a problem hiding this comment.
Just using this comment to provide some positive feedback: kudos on the quick pickup of both vue and nuxt 😄
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds in a new page to display a map with location of all relays using MapBoxGL. Both themes are compatible with the map (light and dark mode)