-
Notifications
You must be signed in to change notification settings - Fork 87
Feature/jss 78 move our contract abis out of the constants package #963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Feature/jss 78 move our contract abis out of the constants package #963
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors contract ABI imports to use scoped subpath exports from @lit-protocol/contracts instead of the root package import, reducing bundle sizes by only including necessary network artifacts (datil/datil-dev/datil-test). A verification script is added to validate that bundles contain only the intended contract artifacts.
Key changes:
- Switched from root
@lit-protocol/contractsimport to scoped subpath imports for specific networks - Added bundle verification script to audit included contract artifacts
- Removed caret from contracts dependency version for stricter version control
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| verify-contracts-bundle.sh | New script that bundles test entries for constants and contracts-sdk packages, then analyzes which contract artifacts are included |
| packages/constants/src/lib/constants/mappers.ts | Refactored to import network-specific contract contexts via subpath exports instead of root import |
| package.json | Changed @lit-protocol/contracts dependency from ^0.0.74 to 0.0.74 (removed caret) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| import { datil as datilContext } from '@lit-protocol/contracts/prod/datil.js'; | ||
| // @ts-ignore -- see note above. | ||
| import { datilDev as datilDevContext } from '@lit-protocol/contracts/prod/datil-dev.js'; | ||
| // @ts-ignore -- see note above. | ||
| import { datilTest as datilTestContext } from '@lit-protocol/contracts/prod/datil-test.js'; | ||
|
|
||
| type DatilContext = typeof datilContext; | ||
| type DatilDevContext = typeof datilDevContext; | ||
| type DatilTestContext = typeof datilTestContext; | ||
|
|
||
| const datil: DatilContext = datilContext; | ||
| const datilDev: DatilDevContext = datilDevContext; | ||
| const datilTest: DatilTestContext = datilTestContext; | ||
|
|
Copilot
AI
Oct 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These re-assignments from imported contexts to new constants appear unnecessary. The imports could be renamed directly using import { datil as datilContext } from ... and then used as datilContext throughout the code, or imported as datil directly to maintain the existing variable names. The current approach adds extra indirection without clear benefit.
| import { datil as datilContext } from '@lit-protocol/contracts/prod/datil.js'; | |
| // @ts-ignore -- see note above. | |
| import { datilDev as datilDevContext } from '@lit-protocol/contracts/prod/datil-dev.js'; | |
| // @ts-ignore -- see note above. | |
| import { datilTest as datilTestContext } from '@lit-protocol/contracts/prod/datil-test.js'; | |
| type DatilContext = typeof datilContext; | |
| type DatilDevContext = typeof datilDevContext; | |
| type DatilTestContext = typeof datilTestContext; | |
| const datil: DatilContext = datilContext; | |
| const datilDev: DatilDevContext = datilDevContext; | |
| const datilTest: DatilTestContext = datilTestContext; | |
| import { datil } from '@lit-protocol/contracts/prod/datil.js'; | |
| // @ts-ignore -- see note above. | |
| import { datilDev } from '@lit-protocol/contracts/prod/datil-dev.js'; | |
| // @ts-ignore -- see note above. | |
| import { datilTest } from '@lit-protocol/contracts/prod/datil-test.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like you prob need the typedefs here, and copilot is wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very excited to see this!! thx for getting this in @Ansonhkg - will be a big improvement for devs. up to you if you wanna implement any of the copilot changes or not. the only one i saw that might make sense is https://github.com/LIT-Protocol/js-sdk/pull/963/files#r2441514234 but i don't think it really matters, just might be a little cleaner, but whatevs
WHAT
@lit-protocol/contractsexport map (legacy root import left commented for reference)verify-contracts-bundle.shscript to inspect bundle size forconstantsandcontracts-sdkpcakges for browser + node targetsAlso see: #962 (comment)
TEST
NPM
A snapshot has been published to
@lit-protocol/[email protected]Test repo: https://github.com/LIT-Protocol/pr-962-bundle-size-test
Before
https://test-962-contracts-sdk-bundle-size.vercel.app/bundle-report-lit-sdk.html
After
https://test-962-contracts-sdk-bundle-size.vercel.app/bundle-report-contracts-sdk-new.html
Locally
./verify-contracts-bundle.sh
BEFORE
AFTER