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

Flow Rust SDK Milestone 4 #42

Merged
merged 4 commits into from
Oct 20, 2021
Merged

Flow Rust SDK Milestone 4 #42

merged 4 commits into from
Oct 20, 2021

Conversation

MarshallBelles
Copy link
Contributor

@MarshallBelles MarshallBelles commented Sep 24, 2021

Flow-Rust-SDK - Milestone 4 [WIP]

This PR is for issue #20.

Current Status

The Flow-Rust-SDK is feature complete, meaning that as a user you can accomplish any task you need to do.
Documentation is almost completed.

Milestones

  • 1: Implement the gRPC layer of the SDK, allowing communication with the Flow blockchain
  • 2: Accomplish transaction signing in a way that handles the complex algorithm / hashing / encoding for the user.
  • 3: Meet and exceed Flow SDK guidelines
  • 4: Complete documentation and common usage examples are available

Authors include:

@marshallbelles
@bluesign

@srinjoyc
Copy link
Contributor

Hi @MarshallBelles - for tracking purposes, would you mind splitting this up into 3 PRs corresponding to the milestones? If possible, point to the relevant repo (potentially specific directories) on how those milestones have been met.

@turbolent
Copy link
Member

@MarshallBelles Great work! I had a look at https://github.com/MarshallBelles/flow-rust-sdk/blob/release/src/lib.rs and you are off to a great start 👍

As far as I can tell, it currently looks like each of the functions making a network request (e.g. get_account) create a new client. It might be more efficient to have these functions reuse a client, i.e. make those functions accept a client instead.

@MarshallBelles
Copy link
Contributor Author

@MarshallBelles Great work! I had a look at https://github.com/MarshallBelles/flow-rust-sdk/blob/release/src/lib.rs and you are off to a great start 👍

As far as I can tell, it currently looks like each of the functions making a network request (e.g. get_account) create a new client. It might be more efficient to have these functions reuse a client, i.e. make those functions accept a client instead.

Very good point. This will also allow the developer to manage their own connection pools.

@sideninja
Copy link
Contributor

@MarshallBelles Great Rust library. I have two pieces of feedback. First is an architectural improvement, where you could extract networking functionality into another layer only exposing the API which would also allow you to swap the implementation with mock for testing. Not a high priority but a possible improvement. The second is about the documentation, although I really like the documentation you already are making I would keep that more as a reference whereas documentation that would be included in our docs page should be shaped in another format. I will provide you with examples and templates for that format before you finish.

@MarshallBelles
Copy link
Contributor Author

@sideninja I’m all ears. Documentation is my weak point. Networking should be abstracted into another layer, I totally agree.

@srinjoyc srinjoyc requested review from orodio and sideninja October 13, 2021 15:21
@sideninja
Copy link
Contributor

@sideninja I’m all ears. Documentation is my weak point. Networking should be abstracted into another layer, I totally agree.

@MarshallBelles Sorry for taking longer (I got sick) but expect documentation templates in the beginning of next week, so you will be able to just provide code examples and that will complete the docs part.

@MarshallBelles
Copy link
Contributor Author

@sideninja I’m all ears. Documentation is my weak point. Networking should be abstracted into another layer, I totally agree.

@MarshallBelles Sorry for taking longer (I got sick) but expect documentation templates in the beginning of next week, so you will be able to just provide code examples and that will complete the docs part.

Documentation templates would be very helpful! I'm planning on using the docs.rs simply as an API reference. I will probably utilize the Github Wiki for tutorials and usage instructions.

@MarshallBelles
Copy link
Contributor Author

MarshallBelles commented Oct 15, 2021

Hi @MarshallBelles - for tracking purposes, would you mind splitting this up into 3 PRs corresponding to the milestones? If possible, point to the relevant repo (potentially specific directories) on how those milestones have been met.

I'll try to get this done sometime next week.

@MarshallBelles MarshallBelles changed the title Flow-Rust-SDK Flow Rust SDK Milestone 4 Oct 17, 2021
@MarshallBelles
Copy link
Contributor Author

@srinjoyc the milestones have been split up into different PRs as requested.

#65

#66

#67

and then this one will be for Milestone 4 [WIP]

@srinjoyc srinjoyc merged commit 2dc20cf into onflow:main Oct 20, 2021
@srinjoyc
Copy link
Contributor

@MarshallBelles I accidentally merged in this PR as I was merging your milestone 1 and 2 that were approved. I ended up having to delete the submission file for milestone 4 after - would you mind re-opening another one for Milestone 4 once you are complete? Sorry for the inconvenience.

@MarshallBelles
Copy link
Contributor Author

MarshallBelles commented Oct 20, 2021 via email

@sideninja
Copy link
Contributor

@MarshallBelles the new documentation template can be found at https://github.com/onflow/sdks/tree/main/templates/documentation

@MarshallBelles
Copy link
Contributor Author

MarshallBelles commented Oct 27, 2021 via email

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.

4 participants