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

refactor: move token-bridge-sdk inside the ui package #752

Merged
merged 16 commits into from
Apr 28, 2023

Conversation

dewanshparashar
Copy link
Contributor

@dewanshparashar dewanshparashar commented Apr 25, 2023

Closes #611

  • Move token-bridge-sdk as is, inside the UI code
  • Use path alias to resolve token-bridge-sdk imports to the folder instead of the package, to minimise import diffs
  • Later refactor inside/out the [ui+token-bridge-sdk] at our own pace
  • All tests and checks should pass

@cla-bot cla-bot bot added the cla-signed label Apr 25, 2023
@vercel
Copy link

vercel bot commented Apr 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
arbitrum-token-bridge ✅ Ready (Inspect) Visit Preview Apr 28, 2023 9:56am

@@ -1,4 +1,4 @@
import { Transaction, AssetType } from 'token-bridge-sdk'
import { Transaction, AssetType } from '../../../src/token-bridge-sdk'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

token-bridge-sdk alias import was not getting correctly resolved in e2e/tests. Probably because of multiple tsconfig.json at each level. Tried redefining the alias in different tsconfigs but couldn't catch a break.

Copy link
Member

@fionnachan fionnachan left a comment

Choose a reason for hiding this comment

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

Generally looks good
2 comments concerning some minor changes
I'd prefer we keep the changes as minimal as possible to avoid unexpected bugs stemming from subtle changes

@@ -11,7 +11,7 @@ import { PropsWithChildren } from 'react'
import { MultiCaller } from '@arbitrum/sdk'

// Create a new cache for every test
const Container = ({ children }: PropsWithChildren<{}>) => (
const Container = ({ children }: PropsWithChildren<any>) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use unknown instead of any here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

export * from './hooks/arbTokenBridge.types'
export * from './hooks/useBalance'
export * from './hooks/useTransactions'
export * from './util/index'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we want to export everything compared to exporting only the methods that are used on token bridge (like we've had it so far)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted now to keep the diffs minimal.

Copy link
Contributor Author

@dewanshparashar dewanshparashar left a comment

Choose a reason for hiding this comment

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

Addressed review pointers.

export * from './hooks/arbTokenBridge.types'
export * from './hooks/useBalance'
export * from './hooks/useTransactions'
export * from './util/index'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted now to keep the diffs minimal.

@@ -11,7 +11,7 @@ import { PropsWithChildren } from 'react'
import { MultiCaller } from '@arbitrum/sdk'

// Create a new cache for every test
const Container = ({ children }: PropsWithChildren<{}>) => (
const Container = ({ children }: PropsWithChildren<any>) => (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@spsjvc spsjvc changed the title refactor: move token-bridge-sdk inside UI package refactor: move token-bridge-sdk inside the ui package Apr 28, 2023
@spsjvc spsjvc self-requested a review April 28, 2023 10:37
@spsjvc spsjvc merged commit cee26dd into master Apr 28, 2023
12 checks passed
@spsjvc spsjvc deleted the refactor-remove-sdk-2 branch April 28, 2023 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move files from token-bridge-sdk to arb-token-bridge-ui
4 participants