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 Swift SDK milestone-1, 2, 3, 4 #63

Merged
merged 1 commit into from
Nov 4, 2021
Merged

Flow Swift SDK milestone-1, 2, 3, 4 #63

merged 1 commit into from
Nov 4, 2021

Conversation

lmcmz
Copy link
Contributor

@lmcmz lmcmz commented Oct 12, 2021

Flow-Swift-SDK - Milestone 1, 2, 3, 4

This PR is for issue #20.

SDK repo

https://github.com/zed-io/flow-swift

Current Status

The flow-swift-SDK has completed all the essential part with test cases and example code. Some documentation has been added in the repo, we will add more documentation in the future. Moreover, we have created other swift SDK, which target for dapp and wallet app usage, fcl-swift and flow-wallet-kit which are dependent on flow-swift. However, those two SDKs are still working in progress.

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

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

Authors include:

@ryankopinsky
@bluesign
@lmcmz

@sideninja
Copy link
Contributor

@lmcmz can I ask you to open a PR on your repo so I can give some comments or feedback, if there is no PR I can't comment.

The first general comment is about the documentation. I will provide you with a documentation template that you can use to fill out with swift code examples, this way the docs will be fully written without a lot of effort from your side. However it is highly recommended to document at least the client API functions so a specification can be autogenerated and a tool like jazzy can be used to generate specifications as a reference for others.

@sideninja sideninja self-requested a review October 18, 2021 13:00
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.

This is a great library with a great design. I don't have any major problems besides the documentation issues I have explained above and some minor questions/feedback, that I will share here.

Since there isn't a PR open I will paste links to the source.

https://github.com/zed-io/flow-swift/blob/main/Sources/Flow/Models/FlowTransaction.swift#L341
Should this be moved from models to a network layer? I think that would make sense because then the models would be API agnostic, whereas now they have implementation details for the grpc API.

https://github.com/zed-io/flow-swift/blob/main/Sources/Flow/Models/FlowTransaction.swift#L199
Shouldn't this actually be checking for equality == instead of >=? If the transaction goes to executed phase it shouldn't be pending. Similar question to some other comparison here.

https://github.com/zed-io/flow-swift/blob/main/Sources/Flow/Models/FlowId.swift#L43
maybe a bit confusing the first argument is in ms since the second is in seconds (subjective and minor).

Again great library and these are mostly minor things so I think after the docs are updated according to the examples I will sent it's good to go 🚀

@lmcmz
Copy link
Contributor Author

lmcmz commented Oct 19, 2021

Hi @sideninja

Sorry for late reply. First of all, thanks for your feedback! Those are really great. 😻
Yeah, you are right. We need more documentation for the SDK.
I will try out the jezz to auto-generate the doc, but yeah if you could send me some template that will be great.

As for the 3 questions you list above, let me give what I think:

https://github.com/zed-io/flow-swift/blob/main/Sources/Flow/Models/FlowTransaction.swift#L341

  1. Should this be moved from models to a network layer?
    Since the gRPC code is auto generate, it already have the Flow_Entities_Transaction.Signature, I wanna have more controlling for my flow entity object so I create my own struct Flow.TransactionSignature. It need some converting between those two struct. 🤔I think keeping Flow.TransactionSignature in model is fine since it's close to business logic.

https://github.com/zed-io/flow-swift/blob/main/Sources/Flow/Models/FlowTransaction.swift#L199
2.Shouldn't this actually be checking for equality == instead of >=?

For those code, I was refer to fcl-js library. I guess it will be better to keep it consistent .
here is the equivalent cod in fcl-js:
https://github.com/onflow/fcl-js/blob/6818103ac65877873f0c62fcee119115783284f9/packages/fcl/src/transaction/index.js#L22-L27

  1. https://github.com/zed-io/flow-swift/blob/main/Sources/Flow/Models/FlowId.swift#L43
    Yes, you are right. I will refactor this part. Nice catch!⚾️

@sideninja
Copy link
Contributor

Hi @sideninja

Sorry for late reply. First of all, thanks for your feedback! Those are really great. 😻 Yeah, you are right. We need more documentation for the SDK. I will try out the jezz to auto-generate the doc, but yeah if you could send me some template that will be great.

As for the 3 questions you list above, let me give what I think:

https://github.com/zed-io/flow-swift/blob/main/Sources/Flow/Models/FlowTransaction.swift#L341

  1. Should this be moved from models to a network layer?
    Since the gRPC code is auto generate, it already have the Flow_Entities_Transaction.Signature, I wanna have more controlling for my flow entity object so I create my own struct Flow.TransactionSignature. It need some converting between those two struct. 🤔I think keeping Flow.TransactionSignature in model is fine since it's close to business logic.

https://github.com/zed-io/flow-swift/blob/main/Sources/Flow/Models/FlowTransaction.swift#L199 2.Shouldn't this actually be checking for equality == instead of >=?

For those code, I was refer to fcl-js library. I guess it will be better to keep it consistent . here is the equivalent cod in fcl-js: https://github.com/onflow/fcl-js/blob/6818103ac65877873f0c62fcee119115783284f9/packages/fcl/src/transaction/index.js#L22-L27

  1. https://github.com/zed-io/flow-swift/blob/main/Sources/Flow/Models/FlowId.swift#L43
    Yes, you are right. I will refactor this part. Nice catch!⚾️

Hi, no worries. I was reading through the SDK and I must say it's really well written so I didn't really find lots of things to improve. Great work.
I will provide a template by the end of the day and instructions how to fill it up.

@lmcmz
Copy link
Contributor Author

lmcmz commented Oct 19, 2021

Hi, no worries. I was reading through the SDK and I must say it's really well written so I didn't really find lots of things to improve. Great work. I will provide a template by the end of the day and instructions how to fill it up.

Thanks a lot @sideninja , we have started investigating jazzy as you mentioned. It looks amazing. We will try to get it done in this week. 😋

@sideninja
Copy link
Contributor

Hi, no worries. I was reading through the SDK and I must say it's really well written so I didn't really find lots of things to improve. Great work. I will provide a template by the end of the day and instructions how to fill it up.

Thanks a lot @sideninja , we have started investigating jazzy as you mentioned. It looks amazing. We will try to get it done in this week. 😋

Great, glad you like it, feel free to use any alternatives tho and you don't need to go overboard with specs if all API methods have specs that's sufficient.

@sideninja
Copy link
Contributor

sideninja commented Oct 20, 2021

@lmcmz doing some more reading of the source code I think I've found a non-trivial issue. Transaction signing is done by using the sign method which is a great approach (I even think I will also adopt in Go SDK) as it hides the implementation complexity of signing payload vs signing envelope, BUT, user should be able to also do those two steps separately if they decide so. Imagine multiple party signatures where signing is done in multiple steps, although you can argue you can still just expose the sign method to support multiple invocation with different signers as arguments (and I even think that is a great argument) the fact is the current implementation of the sign method doesn't allow that if I'm not mistaken.

@sideninja
Copy link
Contributor

Hi @lmcmz here is documentation template you should use for documenting your SDK https://github.com/onflow/sdks/tree/main/templates/documentation

@lmcmz
Copy link
Contributor Author

lmcmz commented Oct 21, 2021

Hi @sideninja , Good point, you are right. The current implementation doesn't have the ability to separate those two steps. I will make a fix for it. But yeah, this SDK is lacking in usage in other projects. It needs more feedback to make it more accessible and easy to use in real business scenarios.

By the way, we have deployed the jazzy here. We will add more documents to it. 😝
https://zed-io.github.io/flow-swift/

@sideninja
Copy link
Contributor

By the way, we have deployed the jazzy here. We will add more documents to it. 😝 https://zed-io.github.io/flow-swift/
Looks awesome!

@sideninja
Copy link
Contributor

@lmcmz after the docs are done this is good to go in my opinion.

@kerrywei kerrywei merged commit 3403e2b into onflow:main Nov 4, 2021
@kerrywei
Copy link

kerrywei commented Nov 4, 2021

merge in the PR per Gregor's comment

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