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 - milestone 2-3-4 #101

Merged
merged 1 commit into from
Nov 12, 2021
Merged

Conversation

nduplessis
Copy link
Contributor

New Tool: Build a Flow SDK - Milestone 4

Description

This PR is for issue #20.

This version of the Ruby SDK provides gRPC communication and a number of the SDK user stories, including script execution and single sig transaction signing and sending.

Usage documentation is available at https://github.com/glucode/flow_client

Milestone 4

All user stories from the SDK guidelines have been completed and tests have been added that execute against the emulator.

Usages documentation can be found at https://github.com/glucode/flow_client

Features

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
  • create a script that returns complex structure 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 with same payer, proposer and authorizer
  • sign a transaction with different payer and proposer
  • sign a transaction with different authorizers using sign method multiple times
  • submit a signed transaction
  • sign a transaction with arguments and submit it

@kerrywei
Copy link

kerrywei commented Nov 1, 2021

Thanks for the submission @nduplessis .
Before my teammate @sideninja take a closer look, the fisr feedback is that we have a sdk documentation template https://github.com/onflow/sdks/blob/main/templates/documentation/template.md which aims to simplify & unify documentation and we encourage all SDKs to use it.

@kerrywei kerrywei requested a review from sideninja November 1, 2021 18:47
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.

Good work on the implementation, however, I have some comments.

I think you should implement your own models for resources like blocks, collections, transactions etc so you can return an instance of such a model instead of returning raw access API responses. There are many benefits in doing so.
Examples:
https://github.com/glucode/flow_client/blob/b9713462681119c15d8a914dc3fd7252722b3078/lib/flow_client/client.rb#L239
https://github.com/glucode/flow_client/blob/b9713462681119c15d8a914dc3fd7252722b3078/lib/flow_client/client.rb#L262

Not doing so causes weird unpacking syntaxes like seen here:
https://github.com/glucode/flow_client/blob/b9713462681119c15d8a914dc3fd7252722b3078/lib/flow_client/client.rb#L213

You could implement cadence models, so instead of passing raw cadence arguments, you could use native implemented cadence types.
Examples:
https://github.com/glucode/flow_client/blob/b9713462681119c15d8a914dc3fd7252722b3078/spec/lib/flow_client/transaction_spec.rb#L277

As mentioned please follow our documentation template and provide it in a file md/mdx in the repository.

Supporting contracts with account creation might be a good idea. Are there good reasons as to why you avoided that?
https://github.com/glucode/flow_client/blob/main/lib/flow_client/client.rb#L62

@nduplessis
Copy link
Contributor Author

nduplessis commented Nov 2, 2021

Thanks @sideninja

I've started on many of these items already and will merge them asap

@sideninja
Copy link
Contributor

Thanks @sideninja

I've started on many of these items all ready and will merge them asap

Great. You have by the end of this week to provide improvements based on the feedback. Let me know if you will have any questions.

@kerrywei
Copy link

kerrywei commented Nov 4, 2021

@nduplessis Let us know when you have updates for review!
Just a reminder: Nov 5th Friday might be the last day for our engineers to provide feedback (though you can for sure to continue iterate based on the feedback until Nov 7). Please take advantage of today and tomorrow to communicate with us more regarding project update. Looking forward to see your improvements!

@nduplessis
Copy link
Contributor Author

Thanks for the heads up @kerrywei. Can you give me an idea which timezone the engineers are in as well by any chance?

@nduplessis
Copy link
Contributor Author

@kerrywei @sideninja I've pushed my latest changes with the models as suggested. If you want to take a look you can do so on master branch. I'm working on the Cadence params and will add the ability to add contracts upon new account creation and will have that merge by Sunday

@sideninja
Copy link
Contributor

@kerrywei @sideninja I've pushed my latest changes with the models as suggested. If you want to take a look you can do so on master branch. I'm working on the Cadence params and will add the ability to add contracts upon new account creation and will have that merge by Sunday

Great, looking good. I will keep an eye on upcoming changes.

@nduplessis nduplessis changed the title Added issue 20 - milestone 4 - nduplessis New Tool: Build a Flow SDK - milestone 4 - nduplessis Nov 7, 2021
@nduplessis nduplessis changed the title New Tool: Build a Flow SDK - milestone 4 - nduplessis New Tool: Build a Flow SDK - milestone 2-3-4 Nov 7, 2021
@sideninja
Copy link
Contributor

I'm having a bit of hard time running some things due to M1 I guess, any suggestions there? Looks like grpc issue.
Error:

because it is not a compatible arch - /Library/Ruby/Gems/2.6.0/gems/grpc-1.41.1-universal-darwin/src/ruby/lib/grpc/2.6/grpc_c.bundle (LoadError)

@nduplessis
Copy link
Contributor Author

Hey @sideninja, yeah that's probably M1 related. I'm looking into this for you and will revert soonest

@nduplessis
Copy link
Contributor Author

@sideninja this does indeed appear to be a pretty widespread issue: grpc/grpc#24846 (comment)

Sorry for not catching this, but unfortunately Apple Silicon is not available on most CI services yet. I'm quickly trying to see if I can rent an M1 in the cloud and will look for a solution

@sideninja
Copy link
Contributor

@sideninja this does indeed appear to be a pretty widespread issue: grpc/grpc#24846 (comment)

Sorry for not catching this, but unfortunately Apple Silicon is not available on most CI services yet. I'm quickly trying to see if I can rent an M1 in the cloud and will look for a solution

No worries, you can ping me when you want to test.

@nduplessis
Copy link
Contributor Author

@sideninja OK so the grpc gem isn't in a great state for Apple Silicon at this point. I will need to do some more work to get the published gem to work correctly.

In the meantime you should be able to use it locally if you clone the repo, but you'll need to use Ruby 2.7.2 as a minimum.
This is pretty easy to install using rbenv

brew install rbenv ruby-build
rbenv install 2.7.2
rbenv local 2.7.2

Install the dependencies

bundle install

Run the unit tests

docker-compose up
bundle exec rspec

Use the gem in interactive ruby

irb -Ilib -rflow_client 

Let me know if this works for you please?

@kerrywei
Copy link

merge in the PR as milestones have been achieved

@kerrywei kerrywei merged commit cf48473 into onflow:main Nov 12, 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