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

Mariari/sdk folder #1868

Merged
merged 9 commits into from
Sep 25, 2023
Merged

Mariari/sdk folder #1868

merged 9 commits into from
Sep 25, 2023

Conversation

mariari
Copy link
Member

@mariari mariari commented Sep 6, 2023

Describe your changes

Indicate on which release or other PRs this topic is based on

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

Copy link
Contributor

@sug0 sug0 left a comment

Choose a reason for hiding this comment

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

this is a step in the right direction, but it should be a separate crate altogether. shared should import the sdk crate, so it's actually decoupled at the dependency level. the sdk should be a leaner crate, and therefore easier+faster to build.

another thing is, I think you should wait for v0.22.0, which is a huge release (it has like a 17k loc diff)

@mariari
Copy link
Member Author

mariari commented Sep 6, 2023

this is a step in the right direction, but it should be a separate crate altogether. shared should import the sdk crate, so it's actually decoupled at the dependency level. the sdk should be a leaner crate, and therefore easier+faster to build.

another thing is, I think you should wait for v0.22.0, which is a huge release (it has like a 17k loc diff)

No that is too much effort. vm relies upon some of the functions that belong in the sdk which is apart of shared. After battling it for a day, I decided to just do it in the current crate, where anything apart of sdk is apart of the SDK itself.

If I were to go down that route, then we'd end up splitting shared into 3 crates

  1. the crate that doesn't call into SDK
  2. SDK
  3. the crate with modules like vm that call into the SDK

For the v0.22.0 release I agree, this is to also get a read on if I'm missing anything from what is considered the "SDK", it shouldn't be too hard to port these changes forward, I hope.

The SDK folder makes it concerete on what modules belong in the public
facing API. RPC is apart of it and is thus moved to the SDK folder.
We also update all imports to properly refer to the new tx location
@mariari
Copy link
Member Author

mariari commented Sep 6, 2023

I've rebased it on v0.22.0 as I wanted to bring in other changes

This type is used extensively in the SDK, and thus it should be
considered under the SDK banner
Wallet is used quite heavily within the SDK for inputs, thus making it
apart of the SDK
@mariari mariari force-pushed the mariari/sdk-folder branch 2 times, most recently from d973bed to a6d23b2 Compare September 7, 2023 15:57
@mariari mariari marked this pull request as ready for review September 7, 2023 15:59
this type is used quite extensively in the SDK and the shared crate in
general, any change to it should be noted in the changelog under SDK
changes as it is developer facing
@mariari mariari mentioned this pull request Sep 11, 2023
@brentstone brentstone merged commit d8c2287 into main Sep 25, 2023
12 checks passed
@brentstone brentstone deleted the mariari/sdk-folder branch September 25, 2023 17:19
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.

5 participants