Skip to content
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

Feature/lit 3549 js sdk add datiltest support #523

Open
wants to merge 34 commits into
base: master
Choose a base branch
from

Conversation

Ansonhkg
Copy link
Collaborator

@Ansonhkg Ansonhkg commented Jul 5, 2024

Description

Add support for datil-test

npm tag

@datil-test

Test

❗️ All tests passed except wrapped keys tests, we need to add datil-test support in the following services:
✅ (11 Jul 24) All tests related to these services now works!

  TestNetworks: 'https://test.wrapped.litprotocol.com/encrypted',
  Production: 'https://wrapped.litprotocol.com/encrypted',
NETWORK=datil-test yarn test:local

@Ansonhkg Ansonhkg changed the base branch from master to hwrdtm/datil-dev-support July 5, 2024 15:42
@Ansonhkg Ansonhkg changed the base branch from hwrdtm/datil-dev-support to feature/3404-datil-dev-support July 5, 2024 15:43
Base automatically changed from feature/3404-datil-dev-support to master July 8, 2024 19:57
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

I've shipped the update to wrapped-keys backend so it supports datil-test now 👍

Looking at how 'give me an RPC URL for network X' logic is become more complex, and it spans across so many packages, I wonder if it would make sense to create a map of network names to RPC urls in constants and reference it there instead of using || statements checking specific network names -- basically the same pattern I used here:

type NETWORK_TYPES = 'TestNetworks' | 'Production';
const SERVICE_URL_BY_NETWORKTYPE: Record<NETWORK_TYPES, string> = {
TestNetworks: 'https://test.wrapped.litprotocol.com/encrypted',
Production: 'https://wrapped.litprotocol.com/encrypted',
};
export const SERVICE_URL_BY_LIT_NETWORK: Record<SupportedNetworks, string> = {
[LitNetwork.DatilDev]: SERVICE_URL_BY_NETWORKTYPE.TestNetworks,
[LitNetwork.DatilTest]: SERVICE_URL_BY_NETWORKTYPE.TestNetworks,
[LitNetwork.Cayenne]: SERVICE_URL_BY_NETWORKTYPE.TestNetworks,
[LitNetwork.Manzano]: SERVICE_URL_BY_NETWORKTYPE.TestNetworks,
[LitNetwork.Habanero]: SERVICE_URL_BY_NETWORKTYPE.Production,
};

WDYT?

I just added one question about the staging.apis domain vs. lit-general-worker -- otherwise this looks good to me.

packages/contracts-sdk/src/lib/contracts-sdk.ts Outdated Show resolved Hide resolved
@Ansonhkg
Copy link
Collaborator Author

Ansonhkg commented Jul 11, 2024

I've shipped the update to wrapped-keys backend so it supports datil-test now 👍

Looking at how 'give me an RPC URL for network X' logic is become more complex, and it spans across so many packages, I wonder if it would make sense to create a map of network names to RPC urls in constants and reference it there instead of using || statements checking specific network names -- basically the same pattern I used here:

type NETWORK_TYPES = 'TestNetworks' | 'Production';
const SERVICE_URL_BY_NETWORKTYPE: Record<NETWORK_TYPES, string> = {
TestNetworks: 'https://test.wrapped.litprotocol.com/encrypted',
Production: 'https://wrapped.litprotocol.com/encrypted',
};
export const SERVICE_URL_BY_LIT_NETWORK: Record<SupportedNetworks, string> = {
[LitNetwork.DatilDev]: SERVICE_URL_BY_NETWORKTYPE.TestNetworks,
[LitNetwork.DatilTest]: SERVICE_URL_BY_NETWORKTYPE.TestNetworks,
[LitNetwork.Cayenne]: SERVICE_URL_BY_NETWORKTYPE.TestNetworks,
[LitNetwork.Manzano]: SERVICE_URL_BY_NETWORKTYPE.TestNetworks,
[LitNetwork.Habanero]: SERVICE_URL_BY_NETWORKTYPE.Production,
};

WDYT?

I just added one question about the staging.apis domain vs. lit-general-worker -- otherwise this looks good to me.

100%

Update 1:
Tracking this here https://linear.app/litprotocol/issue/LIT-3593/[js-sdk]-add-a-map-of-network-names-to-rpc-url

Update 2:
PR up here: https://github.com/LIT-Protocol/js-sdk/pull/527/files

Update 3:
PR#527 is now merged to this PR.

@Ansonhkg Ansonhkg requested a review from glitch003 July 11, 2024 09:42
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

I think our symbol is backwards from what I see on chainlist -- otherwise this is GTG. Nice work on the constant maps <3 :)

packages/constants/src/lib/constants/constants.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

Almost there I think -- tstLPX/tstLIT is a bit of a mindbender. Just one other comment re: relayerUrl handling & types

packages/misc/src/lib/misc.ts Outdated Show resolved Hide resolved
packages/constants/src/lib/constants/constants.ts Outdated Show resolved Hide resolved
packages/constants/src/lib/constants/constants.ts Outdated Show resolved Hide resolved
Ansonhkg and others added 7 commits July 11, 2024 20:32
Co-authored-by: Daryl Collins <daryl@litprotocol.com>
Signed-off-by: Anson <ansonox@gmail.com>
Co-authored-by: Daryl Collins <daryl@litprotocol.com>
Signed-off-by: Anson <ansonox@gmail.com>
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

2 last changes I think, and a nit. We're almost there :D

local-tests/tests/wrapped-keys/util.ts Outdated Show resolved Hide resolved
packages/core/src/lib/lit-core.ts Show resolved Hide resolved
packages/lit-auth-client/src/lib/lit-auth-client.ts Outdated Show resolved Hide resolved
Ansonhkg and others added 2 commits July 11, 2024 22:13
Co-authored-by: Daryl Collins <daryl@litprotocol.com>
Signed-off-by: Anson <ansonox@gmail.com>
Copy link
Contributor

@MaximusHaximus MaximusHaximus left a comment

Choose a reason for hiding this comment

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

👍 LGTM, I think this is GTG 💖
Nice work <3

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.

None yet

3 participants