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

Migrate clients js to sdk v2 #3980

Conversation

panoel
Copy link
Contributor

@panoel panoel commented Jun 12, 2024

This is the first PR migrating clients/js to sdkV2.
The large line count is due to the package-lock.json file.

I added 2 files under an sdk folder. These files were mainly copies of what is in sdkv1. Over the next PRs related to this PR, the code in this folder will be migrated into the appropriate locations. The future PRs will be on a chain-by-chain conversion over to sdkV2. This will make the future PRs more manageable and deal with code that is missing from sdkV2.

Added jest tests:
PASS src/parse_tests.test.ts (39.068 s)
Info Tests
✓ worm info contract mainnet ethereum TokenBridge (1374 ms)
✓ worm info contract mainnet Bsc NFTBridge (1249 ms)
✓ worm info rpc mainnet Bsc (1126 ms)
✓ worm info wrapped ethereum 0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48 sui (1499 ms)
✓ worm info origin sui 0x5d4b302506645c37ff133b98c4b50a5ae14841659738d6d733d59d0d217a93bf::coin::COIN (1368 ms)
✓ worm info registrations mainnet ethereum TokenBridge -v (1916 ms)
✓ worm evm info -c Bsc -n mainnet -m TokenBridge (2560 ms)
EVM Tests
✓ worm evm address-from-secret 0xcfb12303a19cde580bb4dd771639b0d26bc68353645571a8cff516ab2ee113a0 (1137 ms)
Generate Tests
✓ worm generate registration (2158 ms)
✓ worm generate attestation (2564 ms)
Parse Tests
✓ worm parse base64-1 (1707 ms)
✓ worm parse base64-2 (1275 ms)
✓ worm parse big-payload-3 (1300 ms)
✓ worm parse guardian-set-upgrade-1 (1108 ms)
✓ worm parse nft-bridge-transfer-1 (1210 ms)
✓ worm parse token-bridge-attestation-1 (1095 ms)
✓ worm parse token-bridge-registration-1 (1133 ms)
✓ worm parse token-bridge-transfer-1 (1090 ms)
✓ worm parse token-bridge-transfer-2 (1339 ms)
✓ worm parse token-bridge-transfer-4 (1148 ms)
✓ worm parse token-bridge-upgrade-1 (1088 ms)
✓ worm parse token-bridge-upgrade-2 (1189 ms)
✓ worm parse token-bridge-upgrade-3 (1199 ms)
✓ worm parse token-bridge-upgrade-4 (1173 ms)

Test Suites: 1 passed, 1 total
Tests: 24 passed, 24 total

Copy link
Contributor

@barnjamin barnjamin left a comment

Choose a reason for hiding this comment

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

I didn't try any of these but looks pretty formulaic

There are a number of spots that mix getting contract addresses with .get and not using .get

I'd like the chain names to be suggested and we should have what we need to do it

I'd like to know what things we still need the old SDK for and hope a future pass can simplify the code here so that we can remove some of the chain/platform specific files to do a thing, relying on the abstractions of the sdkv2. If we can't because sdkv2 doesn't provide them, we should consider making it provide them

clients/js/src/algorand.ts Outdated Show resolved Hide resolved
clients/js/src/cmds/info/wrapped.ts Show resolved Hide resolved
clients/js/src/cmds/info/rpc.ts Show resolved Hide resolved
clients/js/src/aptos.ts Show resolved Hide resolved
clients/js/src/chains/sui/submit.ts Outdated Show resolved Hide resolved
clients/js/src/tests.ts.test Outdated Show resolved Hide resolved
clients/js/src/consts/contracts.ts Show resolved Hide resolved
@barnjamin barnjamin self-requested a review July 5, 2024 11:46
barnjamin
barnjamin previously approved these changes Jul 5, 2024
barnjamin
barnjamin previously approved these changes Jul 10, 2024
@panoel panoel force-pushed the migrate_clients_js_to_sdkV2 branch from 4c25a88 to fed7444 Compare July 11, 2024 19:08
@panoel panoel force-pushed the migrate_clients_js_to_sdkV2 branch 2 times, most recently from 8d144ed to 8a76596 Compare July 18, 2024 16:08
@evan-gray evan-gray requested review from barnjamin and removed request for barnjamin July 24, 2024 16:03
Copy link
Contributor

@evan-gray evan-gray left a comment

Choose a reason for hiding this comment

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

Appreciate the grunt work it took to move this forward, though I am hesitant about the nearly 40 remaining @certusone/wormhole-sdk dependencies 😬 but it's a start. Just a few items that would be good to clean up.

Also, is run_info_tests and the corresponding info_tests folder still beneficial with those tests now being covered in jest?

clients/js/src/parse_tests.test.ts Outdated Show resolved Hide resolved
clients/js/src/parse_tests.test.ts Outdated Show resolved Hide resolved
clients/js/src/chains/generic/getOriginalAsset.ts Outdated Show resolved Hide resolved
clients/js/src/chains/generic/provider.ts Outdated Show resolved Hide resolved
clients/js/src/cmds/info/registrations.ts Outdated Show resolved Hide resolved
clients/js/src/cmds/status.ts Outdated Show resolved Hide resolved
clients/js/src/cmds/submit.ts Show resolved Hide resolved
clients/js/src/cmds/transfer.ts Show resolved Hide resolved
clients/js/src/cmds/status.ts Show resolved Hide resolved
clients/js/src/parse_tests.test.ts Outdated Show resolved Hide resolved
@panoel
Copy link
Contributor Author

panoel commented Jul 26, 2024

Appreciate the grunt work it took to move this forward, though I am hesitant about the nearly 40 remaining @certusone/wormhole-sdk dependencies 😬 but it's a start. Just a few items that would be good to clean up.

Also, is run_info_tests and the corresponding info_tests folder still beneficial with those tests now being covered in jest?

The benefit is that one can run the script and it will automatically generate the test output for you.

@panoel panoel requested a review from artursapek July 31, 2024 21:05
@panoel panoel force-pushed the migrate_clients_js_to_sdkV2 branch from c3e5880 to 9486811 Compare July 31, 2024 21:13
@evan-gray evan-gray mentioned this pull request Aug 1, 2024
@panoel panoel merged commit 9f98901 into wormhole-foundation:main Aug 1, 2024
24 checks passed
@panoel panoel deleted the migrate_clients_js_to_sdkV2 branch August 1, 2024 14:46
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