LIVE-2174 Dynamic ERC20 CAL #1288
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 850aa79 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
automation
|
@henrily-ledger Screenshots: ❌
It seems this PR contains screenshots that are different from the base branch.
linux
|
Codecov ReportBase: 47.87% // Head: 45.10% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #1288 +/- ##
===========================================
- Coverage 47.87% 45.10% -2.77%
===========================================
Files 691 635 -56
Lines 30376 26757 -3619
Branches 7968 7273 -695
===========================================
- Hits 14541 12069 -2472
+ Misses 14622 13549 -1073
+ Partials 1213 1139 -74
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
It's seems to work fine now : So it's now accessible by this CDN : |
There was a problem hiding this comment.
- live-common: it seems like we do redundant work at loading erc20? (both in ethereum module erc20 and in the global currencies.ts) => in any case we need to kill this idea of a global "preloadTokens" I think => instead we put this in the preload scope of each bridge that needs to load assets. (and the mock run won't be impacted by this , yet we can implement a test scenario in the mock bridges independently that don't depends on network)
- ledgerjs: some fundamental bug on the previously "get" cache, either we drop the cache or we need a "cache map" as you introduced an extra parameter
| @@ -35,11 +60,11 @@ const asContractAddress = (addr: string) => { | |||
| }; | |||
|
|
|||
| // this internal get() will lazy load and cache the data from the erc20 data blob | |||
| const get: () => API = (() => { | |||
| const get: (erc20SignaturesBlob?: string) => API = (() => { | |||
There was a problem hiding this comment.
the get() was a parameter-less function that literally acts as a lazy evaluated value.
there is now a fundamental problem because introducing a parameter here breaks the internal cache that was inside the self called function. (there is a "global" cache variable that bypass the returned value and independently on if you pass a arg or not so we will have a bug.)
so you may need to have a "cache map" approach. You can use a WeakMap for memory.
There was a problem hiding this comment.
I have try something, but I think i'll need your insight can you review this part pls ?
There was a problem hiding this comment.
after reviewing again, something "feels" wrong on this function signature to be honest. this is no longer a get. a get don't take the parameter in parameter that it's going to return 🤔
There was a problem hiding this comment.
is there a possibility to rename it parse(blob: string) and make this parameter non optional?
or is get() still used somewhere? In that case, get can be implemented by get = () => parse(blob) (where blob is the static import one)
it would be good to split the 2 functions and document each usecase, because the semantic isn't easy here
There was a problem hiding this comment.
So when we fetch data from the s3 bucket it will only use the parse command, otherwise it will use the usual get() and cache it
| @@ -35,11 +60,11 @@ const asContractAddress = (addr: string) => { | |||
| }; | |||
|
|
|||
| // this internal get() will lazy load and cache the data from the erc20 data blob | |||
| const get: () => API = (() => { | |||
| const get: (erc20SignaturesBlob?: string) => API = (() => { | |||
ghost
dismissed
* Dynamic cal * dynamic call script edit * readd spl token * mobile dyn cal * remove spl tokens * cal importer with s3 staging * move to ubuntu latest for aws * cal importer in folder * staging dyn cal * fix caching on staging env dyn cal * dyncal erc20-signatures * Feedback from github * fix lint * staging change role * try-selfhosted * selfrunner for aws * change yalm runner * change yalm runner * reomve runner * update doc * Test + bugfix on integration Co-authored-by: Kévin Lambert <[email protected]> * update lock + add schedule to importer * cal importer with prod env * change find to getCryptoCurrencyById * prod cal importer * prd cal importer * redoing cache * fix: different ref * typing * lint fix * add tojson method * feedback fix * add cache for findERC20Signature * better with save file * cache fix lint * remove cache from dyn cal et add it to static * add return * revert pnpm-lock Co-authored-by: Kévin Lambert <[email protected]>
* Dynamic cal * dynamic call script edit * readd spl token * mobile dyn cal * remove spl tokens * cal importer with s3 staging * move to ubuntu latest for aws * cal importer in folder * staging dyn cal * fix caching on staging env dyn cal * dyncal erc20-signatures * Feedback from github * fix lint * staging change role * try-selfhosted * selfrunner for aws * change yalm runner * change yalm runner * reomve runner * update doc * Test + bugfix on integration Co-authored-by: Kévin Lambert <[email protected]> * update lock + add schedule to importer * cal importer with prod env * change find to getCryptoCurrencyById * prod cal importer * prd cal importer * redoing cache * fix: different ref * typing * lint fix * add tojson method * feedback fix * add cache for findERC20Signature * better with save file * cache fix lint * remove cache from dyn cal et add it to static * add return * revert pnpm-lock Co-authored-by: Kévin Lambert <[email protected]>
* Dynamic cal * dynamic call script edit * readd spl token * mobile dyn cal * remove spl tokens * cal importer with s3 staging * move to ubuntu latest for aws * cal importer in folder * staging dyn cal * fix caching on staging env dyn cal * dyncal erc20-signatures * Feedback from github * fix lint * staging change role * try-selfhosted * selfrunner for aws * change yalm runner * change yalm runner * reomve runner * update doc * Test + bugfix on integration Co-authored-by: Kévin Lambert <[email protected]> * update lock + add schedule to importer * cal importer with prod env * change find to getCryptoCurrencyById * prod cal importer * prd cal importer * redoing cache * fix: different ref * typing * lint fix * add tojson method * feedback fix * add cache for findERC20Signature * better with save file * cache fix lint * remove cache from dyn cal et add it to static * add return * revert pnpm-lock Co-authored-by: Kévin Lambert <[email protected]>
📝 Description
We sometimes need to add erc20 tokens out of a release cycle.
There was some issues since we are using electron with two thread, the renderer and the internal doesn't share the same tokens list so we need to initialize / preload the data on both side.
We also work on a automatic PR to be able to push update from the crypto-assets repository toward an S3 (still WIP waiting for the bucket)
https://github.com/LedgerHQ/ledger-live/actions/runs/3061492352
We also need to work on mobile to be sure everything is working
❓ Context
live-commonlive-desktop✅ Checklist
📸 Demo
🚀 Expectations to reach
Please make sure you follow these Important Steps.
Pull Requests must pass the CI and be internally validated in order to be merged.