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

New Tool: Build a Flow SDK [Rust] - Milestone 1 #37

Merged
merged 1 commit into from
Sep 25, 2021
Merged

New Tool: Build a Flow SDK [Rust] - Milestone 1 #37

merged 1 commit into from
Sep 25, 2021

Conversation

fee1-dead
Copy link
Contributor

Description

This PR is for issue #20.

This PR fulfills the requirements for Milestone 1:

  • Implement gRPC & propose an API based on the user stories in the SDK guidelines with rough implementation details on each method.

Submission Links & Documents

NA

Requirements Check

  • Have have you met the milestone requirements? YES
  • Have you included tests (if applicable)? Not at this milestone.
  • Have you met the contribution guidelines of the repos you have submitted code to (if applicable)? NA

Other Details

  • Is there anything specific you'd like the PoC to know or review for?

Please see the rendered md and rust sdk repository.

  • Are there other references, documentation, or relevant artificats to mention for this PR (ie. external links to justify design decisions, etc.)?

None at the moment.

Copy link
Contributor

@psiemens psiemens left a comment

Choose a reason for hiding this comment

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

Hey @fee1-dead, thanks for the submission.

I really like your gRPC implementation. The helper macros are really powerful.

I've left one minor comment on the user stories, but otherwise looks good!

@fee1-dead A couple other developers have been working on a Rust library. Would you be open to collaborating? If so, I can make an introduction!

@fee1-dead fee1-dead requested a review from psiemens September 24, 2021 14:53
@fee1-dead
Copy link
Contributor Author

A couple other developers have been working on a Rust library. Would you be open to collaborating?

Are you talking about https://github.com/MarshallBelles/flow-rust-sdk? If so, then the answer is no. I did take a look at it, but I think my design works best from the ground up.

I did see some problems with their code, for example not having generic arguments, or worse, not using the expected type at all. (see MarshallBelles/flow-rust-sdk#2) In this example, the URI string is very likely to be a literal "foo" and not something that has been built on the heap, therefore it is not optimal to take a reference to the heap (&String) because you just need to take a reference to a string (&str). A bigger problem I see is that their functions do not reuse a connection. Each request needs to establish a new connection instead of acting upon an existing connection. These problems make it hard to build upon it and I believe it is easier to start from scratch.

@MarshallBelles
Copy link
Contributor

MarshallBelles commented Sep 24, 2021

A couple other developers have been working on a Rust library. Would you be open to collaborating?

Are you talking about https://github.com/MarshallBelles/flow-rust-sdk? If so, then the answer is no. I did take a look at it, but I think my design works best from the ground up.

I did see some problems with their code, for example not having generic arguments, or worse, not using the expected type at all. (see MarshallBelles/flow-rust-sdk#2) In this example, the URI string is very likely to be a literal "foo" and not something that has been built on the heap, therefore it is not optimal to take a reference to the heap (&String) because you just need to take a reference to a string (&str). A bigger problem I see is that their functions do not reuse a connection. Each request needs to establish a new connection instead of acting upon an existing connection. These problems make it hard to build upon it and I believe it is easier to start from scratch.

&String is necessary for thread safety and usability. You're always welcome to fork my code and adjust for your own needs. The project is both MIT and Apache v2.0 so you can choose how you want to use it. The heap reference is really not a big deal unless you are wanting to run on a very low memory device.

@MarshallBelles
Copy link
Contributor

Also, Arguments are handled in the flow-rust-sdk like this:

    let keys_arg = process_keys_args(account_keys);
    let contracts_arg = Argument::dictionary(vec![]);

    let keys_arg = json!(keys_arg);
    let contracts_arg = json!(contracts_arg);

    let transaction: Transaction = build_transaction(
        create_account_template.to_vec(),
        vec![
            serde_json::to_vec(&keys_arg)?,
            serde_json::to_vec(&contracts_arg)?,
        ],
        latest_block.block.unwrap().id,
        1000,
        proposer,
        vec![payer.clone()],
        payer.clone(),
    )
    .await?;

@fee1-dead
Copy link
Contributor Author

&String is necessary for thread safety and usability.

&String is not more thread safe and usable than &str. In fact &String is less usable because every time you need to pass a literal as a &String you need to allocate it on the heap and copy it to the heap first.

The heap reference is really not a big deal unless you are wanting to run on a very low memory device.

I see it as a big deal because it makes the API hard to use in a way, and introduces unnecessary overhead.

&String is a pointer to a structure that contains a pointer, the string's length as well as the capacity.

&str is a pointer with length. Any literal "foo" is of type &str and in reality it points to somewhere in the compiled binary where the string resides. Converting it to &String will need to allocate on the heap and copy the &str's data to the heap, which is not good for performance.

@MarshallBelles
Copy link
Contributor

&String is necessary for thread safety and usability.

&String is not more thread safe and usable than &str. In fact &String is less usable because every time you need to pass a literal as a &String you need to allocate it on the heap and copy it to the heap first.

The heap reference is really not a big deal unless you are wanting to run on a very low memory device.

I see it as a big deal because it makes the API hard to use in a way, and introduces unnecessary overhead.

&String is a pointer to a structure that contains a pointer, the string's length as well as the capacity.

&str is a pointer with length. Any literal "foo" is of type &str and in reality it points to somewhere in the compiled binary where the string resides. Converting it to &String will need to allocate on the heap and copy the &str's data to the heap, which is not good for performance.

I disagree. I find the String much more useful even at the price of heap allocation, But you are free to do as you like. Best of luck to you good sir!

@psiemens
Copy link
Contributor

Looks good, thanks @fee1-dead

@psiemens psiemens merged commit 6fd51ca into onflow:main Sep 25, 2021
@srinjoyc srinjoyc changed the title New Tool: Build a Flow SDK - Milestone 1 New Tool: Build a Flow SDK [Rust] - Milestone 1 Sep 27, 2021
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.

3 participants