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.

Sign up for GitHub

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

Jump to bottom

Replace internal Starknet CLI usage with starknet.js #400

Merged
merged 2 commits into from
Aug 3, 2023
Merged

Replace internal Starknet CLI usage with starknet.js #400

merged 2 commits into from
Aug 3, 2023

Conversation

penovicp
Copy link
Contributor

@penovicp penovicp commented Jul 11, 2023 •
edited by FabijanC
Loading

Usage related changes

  • For happiest path scenarios the hope is that users should not be affected
  • Minor API changes for ancillary functionalities and functions, most notably a truncated response for the getBlock method
  • Some internally re-thrown errors might be different due to differences in the underlying tool used

Development related changes

Checklist:

  • Formatted the code
  • No linter errors + tried to avoid introducing linter warnings
  • Performed a self-review of the code
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Documented the changes
  • Updated the test directory (with a test case consisting of network.json, hardhat.config.ts, check.ts)
  • Linked issues which this PR resolves
  • Created a PR to the plugin branch of starknet-hardhat-example:

FabijanC
FabijanC reviewed Jul 12, 2023
View reviewed changes
Copy link
Collaborator

@FabijanC FabijanC left a comment

Choose a reason for hiding this comment

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

Looks good so far, thank you for the effort. I left one comment and updated the development changes section of the PR description to reflect the starknet.js update, feel free to edit it.

src/account-utils.ts Outdated Show resolved Hide resolved
@penovicp penovicp marked this pull request as ready for review July 26, 2023 11:47
This was referenced Jul 28, 2023
Closed
Open
FabijanC
FabijanC reviewed Aug 1, 2023
View reviewed changes
Copy link
Collaborator

@FabijanC FabijanC left a comment

Choose a reason for hiding this comment

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

What about documentation updates? Are there any? Were you planning on adding them?

src/adapt.ts Show resolved Hide resolved
src/starknet-wrappers.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
@penovicp
Copy link
Contributor Author

penovicp commented Aug 1, 2023

What about documentation updates? Are there any? Were you planning on adding them?

I wasn't planning to since the outward facing API should basically be the same (there are some minor differences for getBlock). It might make sense to note the CLI deprecation somewhere in the dev section of the documentation.

@penovicp penovicp requested a review from FabijanC August 2, 2023 05:15
FabijanC
FabijanC approved these changes Aug 2, 2023
View reviewed changes
Copy link
Collaborator

@FabijanC FabijanC left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Basically ready, just the example repo branch specification needs to be restored after 0xSpaceShard/starknet-hardhat-example#123 is merged.

@penovicp penovicp merged commit 3364baf into master Aug 3, 2023
@penovicp penovicp deleted the stjs branch August 3, 2023 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FabijanC FabijanC FabijanC approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bump starknet.js to v5 Create a starknet.js wrapper
2 participants
@penovicp @FabijanC