Skip to content

Conversation

@Ansonhkg
Copy link
Collaborator

@Ansonhkg Ansonhkg commented Oct 17, 2024

Description

Providing Lit Action global types and convert all Lit actions in the wrapped-keys-lit-actions to Typescript. There are few areas that i'm using : any as this is a first-pass (fixed this). I also realised that the Type definition we got is not completely accurate, so we need to modify it as we see.

note that the Lit Action using [email protected]

update 1:
also console logging out the bundle size:
image

@Ansonhkg Ansonhkg added the ✨ Enhancement New feature or request label Oct 17, 2024
@Ansonhkg Ansonhkg self-assigned this Oct 17, 2024
@Ansonhkg Ansonhkg requested a review from joshLong145 as a code owner October 17, 2024 16:50
Base automatically changed from LIT-3959-raw-wrapped-keys-litaction-functions to master October 18, 2024 14:58
import { ethers } from 'ethers';

// This is weird - ethers.UnsignedTransaction is not the same as the one being used here..
// We should fix this soon, but not a hard blocker
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for visibility

@Ansonhkg
Copy link
Collaborator Author

We also need to test the Lit Actions before merging this - the import { ethers } from "ethers"; line seems to be causing some issues even it's set to be external in the esBuild.config.js
image

@Ansonhkg Ansonhkg added the 🚧 In Progress - Don’t Merge pull request is still under active development and should not be merged into the main branch. It sig label Oct 19, 2024
@Ansonhkg
Copy link
Collaborator Author

Ansonhkg commented Oct 19, 2024

We also need to test the Lit Actions before merging this - the import { ethers } from "ethers"; line seems to be causing some issues even it's set to be external in the esBuild.config.js image

So in the bundled code, we need to manually modify a few things before using, especially when we are using the Lit Action packages (eg. ethers)

if minify is disabled, we need to:

        content = content
          .replace(/var import_ethers = __require\("ethers"\);/g, "")
          .replace(/import_ethers\./g, "");

if minify is enabled, we need to:

        content = content
          .replace(/var\s+\w+=\w+\("ethers"\);/g, "")
          .replace(/[a-zA-Z]\.ethers/g, "ethers");

cc @MaximusHaximus @FedericoAmura

@Ansonhkg
Copy link
Collaborator Author

Ansonhkg commented Oct 19, 2024

nit: And do we always want to bundle the buffer.shim.js in? Including the shim could increase the code size from 0.6144 KB to 29.696 KB, which is quite significant relatively to the actual code, but not urgent to what Lit Action server could handle.

image

in the config file:

inject: ["./buffer.shim.js"],

Maybe we can add some metadata in the Lit Action .ts file to conditionally inject the ./buffer.shim.js file. We can parse the content in the esBuild.config.js file.

@Ansonhkg Ansonhkg removed the 🚧 In Progress - Don’t Merge pull request is still under active development and should not be merged into the main branch. It sig label Oct 22, 2024
- Ran `eslint --fix` too.
- Removed now-unnecessary manual string replace logic from esbuild
- Update type references to use ethers types instead of relying on a reference to an actual imported instance of the classes
…scopes

- Also removed now-unnecessary eslint hints for `/* global Lit */` etc.
- Fixes mismatched type conflict with same name 'UnsignedTransaction'; ethereum's type is different than Solana.
@MaximusHaximus
Copy link
Contributor

MaximusHaximus commented Oct 22, 2024

We also need to test the Lit Actions before merging this - the import { ethers } from "ethers"; line seems to be causing some issues even it's set to be external in the esBuild.config.js image

So in the bundled code, we need to manually modify a few things before using, especially when we are using the Lit Action packages (eg. ethers)

if minify is disabled, we need to:

        content = content
          .replace(/var import_ethers = __require\("ethers"\);/g, "")
          .replace(/import_ethers\./g, "");

if minify is enabled, we need to:

        content = content
          .replace(/var\s+\w+=\w+\("ethers"\);/g, "")
          .replace(/[a-zA-Z]\.ethers/g, "ethers");

cc @MaximusHaximus @FedericoAmura

  • I found a way we can define the ethers global in the new global.d.ts file you made, similarly to how you defined the Lit namespace 👍

    • This approach lets us get rid of the manual string replacements on the bundled code
    • We still get proper type completion in the LIT action source files for ethers 👍
    • See commit here.
  • I also fixed some type issues I ran into while working on some new changes to the LIT actions I'm doing that are based on this branch...

    • Because each LIT action module file was modifying the global namespace, all source files thought all jsParams for any of our LIT actions were valid, global variables, even though they are actually specific to the function being called and might not exist depending on which function's source code we were looking at.
    • The global namespace mutation also caused a bit of a problem when I went to fix the UnsignedTransaction type to be explicit -- it has a different definition for Solana than for Ethereum, but defining them both with the same name in the global namespace caused problems for TS since the types couldn't be assigned to eachother.
    • By declaring them in the self-executing-actions modules, we get the benefit of type-safety on the jsParams without the definitions bleeding into other functions global scope definitions 👍
    • See commit here
  • I also replaced the last few any types with more explicit types...

    • We get type-safe references in a few additional places now
    • There are no eslint errors about disallowed any types in any of the files now
    • See commit here

Nice work on getting this done man 💖 This is LGTM if you're GTG with the changes I made.

cc: @FedericoAmura

@MaximusHaximus
Copy link
Contributor

MaximusHaximus commented Oct 22, 2024

nit: And do we always want to bundle the buffer.shim.js in? Including the shim could increase the code size from 0.6144 KB to 29.696 KB, which is quite significant relatively to the actual code, but not urgent to what Lit Action server could handle.

image > in the config file:
inject: ["./buffer.shim.js"],

Maybe we can add some metadata in the Lit Action .ts file to conditionally inject the ./buffer.shim.js file. We can parse the content in the esBuild.config.js file.

Hmmm -- that sounds like a nice optimization for the simplest functions 🤔 Though, it is pretty nice to not have to think about whether it's needed for a specific function... or need to deal with having to add it when/if we modify a function that doesn't need it now, but starts to due to a change we make.

I think I fall on the 'leave it in for all functions until (if we ever do) we get some push-back on it from consumer(s)' to keep it simpler until we have a requirement to slim the functions down even further, especially given the standard case here is that LIT nodes will be downloading the source on a decent network link -- but I don't have a hugely strong opinion about it...

@Ansonhkg
Copy link
Collaborator Author

Thanks for fixing this up! This LGTM and is good to merge!

@Ansonhkg Ansonhkg merged commit 1e074af into master Oct 23, 2024
4 checks passed
@Ansonhkg Ansonhkg deleted the feat/convert-wrapped-keys-lit-actions-to-ts branch October 23, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

✨ Enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants