Skip to content

Conversation

@AngelCastilloB
Copy link
Member

Context

The CSL was compiled to WASM using an old version of rustwasm which had a bug that would leak stack space when errors were thrown by the underlying rust library, This was making the CSL WASM module to crash after a several errors were thrown, this was fixed in later versions. The CML library uses the updated rustwasm and as such doesnt have this bug.

Proposed Solution

Reemplace the CSL library with the CML library

Copy link
Member

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

I had a heart-in-mouth moment 😆:
image

Did you run npm i? Please ensure yarn install has been run again after removing the package-lock.json

@AngelCastilloB
Copy link
Member Author

I had a heart-in-mouth moment 😆: image

Did you run npm i? Please ensure yarn install has been run again after removing the package-lock.json

addressed here: c5cf29f

@AngelCastilloB AngelCastilloB force-pushed the chore/adp-1858-upgrade-from-csl-to-cml branch from c5cf29f to 2c41895 Compare October 27, 2022 15:19
Copy link
Member

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

The complete diff looks good, but there's a few distinct changes that have been lumped into a single commit, that need to be separated. I've commented on the ones that stick out to me, but it also appears that we missed including some CSL objects in the managed scope the first time around, so they would be best committed first as fix, then the lib replacement can be clean. If the new lib introduces new objects, then that's naturally part of the refactor commit.

@AngelCastilloB AngelCastilloB force-pushed the chore/adp-1858-upgrade-from-csl-to-cml branch 3 times, most recently from 6494cee to b01944c Compare October 28, 2022 04:38
@AngelCastilloB
Copy link
Member Author

The complete diff looks good, but there's a few distinct changes that have been lumped into a single commit, that need to be separated. I've commented on the ones that stick out to me, but it also appears that we missed including some CSL objects in the managed scope the first time around, so they would be best committed first as fix, then the lib replacement can be clean. If the new lib introduces new objects, then that's naturally part of the refactor commit.

I split the extra manage here: b01944c

There are several new .manage calls, but is because now some of the functions that used to return just a number or a byte array now return an object, so those must be manage

Copy link
Member

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

image

Some Spanglish? 😄

reemplaced -> replaced

@rhyslbw
Copy link
Member

rhyslbw commented Oct 28, 2022

I split the extra manage here: b01944c

Great, that's good to see there was only a small number missed in the original implementation

We replaced the CSL with the CML library from our SDK, however, the CSL library is still being loaded by a sub-dependnecy (blockfrost-js), both libraries are heavy and together are increasing the load time of the web-extension.
mkazlauskas
mkazlauskas previously approved these changes Oct 28, 2022
Copy link
Member

@mkazlauskas mkazlauskas left a comment

Choose a reason for hiding this comment

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

Nice work!

Copy link
Member

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

Great work @AngelCastilloB 👷

@rhyslbw rhyslbw merged commit cd6ff0c into master Oct 28, 2022
@rhyslbw rhyslbw deleted the chore/adp-1858-upgrade-from-csl-to-cml branch October 28, 2022 08:19
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.

4 participants