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

dotnet sdk milestone 2 submission #68

Merged
merged 4 commits into from
Nov 5, 2021
Merged

dotnet sdk milestone 2 submission #68

merged 4 commits into from
Nov 5, 2021

Conversation

tyronbrand
Copy link
Contributor

@tyronbrand tyronbrand commented Oct 17, 2021

Flow .Net SDK - Milestone 1, 2

This PR is for issue #20.

SDK repo

https://github.com/tyronbrand/flow.net

Current Status

The flow.net SDK has completed all the essential parts with some tests and example code.

Still to come:

  • Documentation
  • More tests/examples

Blocks:

  • retrieve a block by ID
  • retrieve a block by height
  • retrieve the latest block

Collections:

  • retrieve a collection by ID

Events:

  • retrieve events by name in the block height range

Scripts:

  • submit a script and parse the response
  • submit a script with arguments and parse the response

Accounts:

  • retrieve an account by address
  • create a new account
  • deploy a new contract to the account
  • remove a contract from the account
  • update an existing contract on the account

Transactions:

  • retrieve a transaction by ID
  • sign a transaction (single payer, proposer, authorizer or combination of multiple)
  • submit a signed transaction
  • sign a transaction with arguments and submit it

Milestones

  • 1 [x] Implement the gRPC layer of the SDK, allowing communication with the Flow blockchain
  • 2 [x] 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

@sideninja sideninja self-requested a review October 18, 2021 13:05
@janezpodhostnik janezpodhostnik self-requested a review October 18, 2021 19:34
@janezpodhostnik
Copy link
Collaborator

janezpodhostnik commented Oct 21, 2021

I was testing the SDK out here are some of my comments so far:

  • I suggest replacing concatenating path like this $"{AppContext.BaseDirectory}\\Cadence"; with this Path.Combine(AppContext.BaseDirectory, "Cadence"). I had problems running the examples on a mac because of this :)
  • Have you considered creating a BaseExample class so creating the client doesn't need to be duplicated across all of the examples?
  • looking at the output of the examples, its really hard to see which example is outputting what and why. It would be nice that the name of the example and the description of the output would also be visible in the console output.
  • I like the cadence encoding/decoding code!
  • have you considered publishing a nuget package?

Great work so far! Looks very nice! 👍

@tyronbrand
Copy link
Contributor Author

tyronbrand commented Oct 21, 2021

Thanks for the feedback @janezpodhostnik

With regards to the path issue, I will put in a fix this evening. Good spot!

I will make the changes to the example project that you suggested.

I did publish a package a few hours ago. I have updated the README with the info if you wish to try it out.

Would you like me to close the PR and reopen once the above tasks have been completed?

@sideninja
Copy link
Contributor

Great SDK @tyronbrand, I have a couple of comments.

Documentation:
Good job on documentation but we have a new documentation template available here that you should use to add your examples and links to specification. We did this because we want to unify SDK documentations to look similar between languages and to allow developer to get a better understanding of how things work. It would be required from you to use documentation generator and link to specifications generated from the docs. Let me know if you have any questions or problems there.

Examples could use some more comments.

It would be a good feature to do a check that provided signer is actually supposed to sign the payload or envelope. This is nothing major just a nice UX improvement.
https://github.com/tyronbrand/flow.net/blob/d83ed48f0eb17b4001a38f2cbc7a7c7f498720fa/src/Flow.Net.SDK/Extensions/FlowTransactionExtensions.cs

@tyronbrand
Copy link
Contributor Author

Thanks @sideninja,

I will check out the documentation template now and add more comments to the examples.

Once that's out the way I will look at the signer improvement you suggested.

@tyronbrand
Copy link
Contributor Author

Hey @sideninja & @janezpodhostnik

I have implemented the changes @janezpodhostnik recommended.
Additionally, I have created documentation based on the template @sideninja recommended.

Thanks

Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

Great, thank you for making the changes. Awesome job on the docs !!! also banner looks sleek

Copy link
Collaborator

@janezpodhostnik janezpodhostnik 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! Nice.

@srinjoyc srinjoyc merged commit ad69757 into onflow:main Nov 5, 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.

4 participants