Skip to content

Improvement: assets detection#97

Merged
estebanmino merged 5 commits intomasterfrom
improvement/assets-detection
May 1, 2019
Merged

Improvement: assets detection#97
estebanmino merged 5 commits intomasterfrom
improvement/assets-detection

Conversation

@estebanmino
Copy link
Copy Markdown
Contributor

@estebanmino estebanmino commented Apr 30, 2019

This PR adds some improvements to assets detection:

  1. Use of safelyExecute inside each individual assets detection methods, detectTokens and detectCollectibles. With this we can use these methods directly and handle errors.
  2. Collectibles without image or name are now accepted, following the EIP.
  3. Added a util method, fetchTimeout to handle requests to external APIs.
  4. Use of fetchTimeout in collectibles detection.

Copy link
Copy Markdown
Contributor

@brunobar79 brunobar79 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. Some comments regarding fetch + timeout

try {
/* istanbul ignore if */
if (assetsController.openSeaApiKey) {
response = await timeoutFetch(api, { headers: { 'X-API-KEY': assetsController.openSeaApiKey } }, 2000);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2 seconds it's too low. I'd go with at least 10-15. Remember the app might be used with not so great internet connections, which is usually not a problem in desktop.

Also move it to a constant at the top of the file specifying what it is for.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

good point, totally agree

if (assetsController.openSeaApiKey) {
response = await timeoutFetch(api, { headers: { 'X-API-KEY': assetsController.openSeaApiKey } }, 2000);
} else {
response = await timeoutFetch(api, {}, 2000);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #97 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #97   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          21     21           
  Lines        1408   1412    +4     
  Branches      181    180    -1     
=====================================
+ Hits         1408   1412    +4
Impacted Files Coverage Δ
src/AssetsDetectionController.ts 100% <100%> (ø) ⬆️
src/AssetsController.ts 100% <100%> (ø) ⬆️
src/util.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62f14a0...3df762a. Read the comment docs.

@estebanmino estebanmino merged commit dd736ad into master May 1, 2019
@estebanmino estebanmino deleted the improvement/assets-detection branch May 1, 2019 17:06
mcmire pushed a commit to mcmire/core that referenced this pull request Jul 17, 2023
* 5.0.1

* Update changelog

---------

Co-authored-by: github-actions <github-actions@github.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
* fetch timeout

* handle fetch requests

* ignore name and image condition

* handle tests

* update to 15 secs
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
* fetch timeout

* handle fetch requests

* ignore name and image condition

* handle tests

* update to 15 secs
Mrtenz pushed a commit that referenced this pull request Oct 16, 2025
* Add missing workflows from template repo
* Replace `.gitattributes`, `.editorconfig`, `.gitignore` with version
  from template repo
* Add NPM scripts in template repo but missing here
  * Add `allow-scripts` and run it via `yarn setup`
* Update TypeScript config to match template repo
  * Exclude test files
  * Use ES2020 `.d.ts` instead of ES2017
* Upgrade Node to v14
* Lint JSON files with `eslint-plugin-packagejson` instead of
  `prettier-plugin-json`
* Update README to match template repo
* Update and backfill changelog
Mrtenz pushed a commit that referenced this pull request Oct 16, 2025
…be used (#97)

Co-authored-by: Prem Baranwal <premb@talentica.com>
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.

3 participants