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

sdk: improve type safety + add documentation #1102

Merged
merged 9 commits into from
May 5, 2022
Merged

Conversation

kcsongor
Copy link
Contributor

@kcsongor kcsongor commented Apr 22, 2022

The main goal of this PR is to improve the type safety of some of the sdk functionality.

  • isEVMChain now performs type refinement. This way, we can achieve exhaustivity checking in downstream code that relies on this check.
  • This in turn allows the address conversion functions to have safer types, along with exhaustivity checking. The end result is that when a new chain is added, these functions will raise a type error, indicating that the new chain ids are not handled. No null value is returned from using the functions in an intended way (that is, passing in well-typed arguments). This obviates the need for callers to coalesce null cases to values like "" which results in a debugging nightmare.
  • Added some return types, in particular some of these types were inferred to be things like Promise<any>, which is not that helpful.
  • Added some documentation

This change is Reviewable

@kcsongor kcsongor requested a review from evan-gray April 22, 2022 10:51
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.

Nice uses of never! Three requests:

  1. Can we add some verbose tests for the array utils?
  2. Please update the CHANGELOG when making SDK changes.
  3. This will require a minor version bump. Can you please update the package to reflect this?

sdk/js/src/utils/array.ts Show resolved Hide resolved
sdk/js/src/utils/array.ts Outdated Show resolved Hide resolved
@kcsongor kcsongor force-pushed the sdk-type-improvements branch 5 times, most recently from e7f6df0 to 8d35eaa Compare April 25, 2022 20:01
@evan-gray evan-gray force-pushed the sdk-type-improvements branch from 4bfbb59 to 5c4a6d9 Compare May 3, 2022 23:36
@netlify
Copy link

netlify bot commented May 3, 2022

Deploy Preview for tubular-dango-1656b2 canceled.

Name Link
🔨 Latest commit a6f2b8c
🔍 Latest deploy log https://app.netlify.com/sites/tubular-dango-1656b2/deploys/6273f410e88f270009777151

@netlify
Copy link

netlify bot commented May 3, 2022

Deploy Preview for roaring-moonbeam-aa0ca5 canceled.

Name Link
🔨 Latest commit a6f2b8c
🔍 Latest deploy log https://app.netlify.com/sites/roaring-moonbeam-aa0ca5/deploys/6273f41028fc800008bace46

@evan-gray evan-gray force-pushed the sdk-type-improvements branch 3 times, most recently from 12d38d4 to b040fd0 Compare May 5, 2022 14:29
@evan-gray evan-gray force-pushed the sdk-type-improvements branch from b040fd0 to 986b32a Compare May 5, 2022 14:32
@evan-gray evan-gray force-pushed the sdk-type-improvements branch from 986b32a to 683e733 Compare May 5, 2022 14:50
@evan-gray evan-gray self-requested a review May 5, 2022 14:52
evan-gray
evan-gray previously approved these changes May 5, 2022
sdk/js/src/utils/array.ts Outdated Show resolved Hide resolved
@evan-gray evan-gray requested a review from panoel May 5, 2022 15:06
@evan-gray evan-gray force-pushed the sdk-type-improvements branch from 683e733 to 063655c Compare May 5, 2022 15:37
evan-gray
evan-gray previously approved these changes May 5, 2022
@panoel panoel requested a review from evan-gray May 5, 2022 16:03
@evan-gray evan-gray merged commit 7f42713 into dev.v2 May 5, 2022
@evan-gray evan-gray deleted the sdk-type-improvements branch May 5, 2022 16:35
bruce-riley pushed a commit that referenced this pull request May 25, 2022
* sdk/js: improve type safety + add documentation

* sdk/js: add support for chain names

* sdk/js: fix boolean conditional bug in transferFromSolana

* sdk/js: remove unsafe ChainId coercions in sdk

* sdk/js: Chain id 0

This is a legit chain id that some governance VAAs target

* sdk/js: Add mainnet and testnet contract addresses

* sdk/js: bump version to 0.3.0

* sdk/js: limit minor version changes

* sdk/js: update contracts and add devnet

Co-authored-by: Csongor Kiss <ckiss@jumptrading.com>
Co-authored-by: Evan Gray <battledingo@gmail.com>
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.

3 participants