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

adds bech32 to dependencies for @ironfish/sdk #3608

Merged
merged 1 commit into from
Mar 6, 2023
Merged

Conversation

hughy
Copy link
Contributor

@hughy hughy commented Mar 6, 2023

Summary

yarn add bech32

after installing @ironfish/sdk, bech32 module cannot be found (e.g., https://discord.com/channels/771503434028941353/831945060777852998/1082211129796874311)

Testing Plan

Breaking Change

Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.

[ ] Yes

`yarn add bech32`

after installing @ironfish/sdk, bech32 module cannot be found.
@hughy hughy requested a review from a team as a code owner March 6, 2023 16:16
@hughy
Copy link
Contributor Author

hughy commented Mar 6, 2023

We define util functions that use bech32 in the SDK, but it looks like we only use them in the CLI right now. An alternative to this dependency is to move those util definitions to the CLI.

Copy link
Member

@dguenther dguenther left a comment

Choose a reason for hiding this comment

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

This seems like an adequate quick fix -- I'm not expecting the yarn.lock to need updating since it's already in CLI. Could you test that it fixes the issue with the sdk before merging?

@hughy
Copy link
Contributor Author

hughy commented Mar 6, 2023

Confirmed that this fixes the issue by testing iron-fish/proxy-example#1 by installing @ironfish/sdk locally from this branch

@hughy hughy merged commit 0348e09 into staging Mar 6, 2023
@hughy hughy deleted the fix/bech32-dependency branch March 6, 2023 18:23
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.

2 participants