dotnet sdk milestone 3, 4 submission

[tyronbrand]

Flow-.Net-SDK - Milestone 3, 4

This PR is for issue #20.

SDK repo

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

Current Status

The flow.net SDK has completed all required milestones. This includes, tests, documentation and 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 [x] Meet and exceed Flow SDK guidelines
• 4 [x] Complete documentation and common usage examples are available

[janezpodhostnik]

• on the latest master the examples are commented output

• note on extension methods (personal preference disclaimer):

  I like to use extension methods only as a last resort because its hard to extend them or test them. Things like string Encode(this ICadence cadence) could be part of the cadence interface and implemented int a Cadence abstract base class. Same with AsCadenceType, CadenceCompositeValue and CadenceCompositeValueAsCadenceType

• note on naming (personal preference disclaimer):

  I noticed lines like this cadenceResponse.AsCadenceType<CadenceComposite>().CadenceCompositeValueAsCadenceType<CadenceString>("name").Value This just ahs way to much "cadence" in it :) cadenceResponse is already a ICadence so AsCadenceType could simply be As. CadenceComposite is already in the cadence namespace, so im not sure it needs a Cadence prefix, but possibly yes. CadenceCompositeValueAsCadenceType this one has a lot in the name... I would unpack this as CadenceComposite -> Composite, Value -> Field (because we are casting the field of the composite), AsCadenceType -> As. With this you would get cadenceResponse.As<CadenceComposite>().CompositeFieldAs<CadenceString>("name").Value, which imo looks better and is easier to read.

Summary from my usage experience:

• milestone 3:
  • test pass (but are a bit sparse)
  • examples pass (except the one that is a known problem on the emulator) (but are commented out?)
  • I was able to send scripts and transactions with arguments
  • milestone 3 looks good to me
  • side note, nuget package is a bit stale
• milestone 4:
  • docs look good.
  • code is unintuitive. This is a broad statement so let me explain:
    • code lacks documentation. (Heavy documentation is not needed if naming is very good or if the patterns used are very common.)
    • naming is sometimes hard to decipher or misleading (NewEcdsaAccountKey creates a random key; CadenceCompositeValueAsCadenceType doesnt convert a composite value it converts the fields of the composite)
    • heavy use of extension methods makes things hard to find. I was looking how to sing a transaction, but I was looking in FlowTransaction which does not have that method, because it is an extension method
    • some things could use their own custom type ( account address is a ByteString but could be a dedicated type to make it clearer when methods accept it)
  • lack of constants forces the user to write magic strings/numbers (key weight threshold, in-built event names)
  • There is a lack of custom exceptions (using a FromHexToByteString on a string prefixed by 0x produced a generic exception, which I had to decipher) and some unsafe code (using the return of FirstOrDefault as if it cant be null)
  • overall some effort could still go into milestone 4 to make usage a lot nicer.

My raw notes from me trying to create an account:

• Imported nuget package. Nice!
• FlowClientAsync.Create works great, but not sure if this should not just be a overloaded constructor
• Found docs how to get reference block, worked easy enough
• Some comments on the classes would be very helpful
• A dedicated type for an address instead of directly using a ByteString might make it more obvious which fields should be filled with addresses
• The NewEcdsaAccountKey method generates a random key, it would be good to either rename the method or comment on it
• The 1000 key weight should be a constant somewhere
• It would be nice if FromHexToByteString handled 0x prefixes. It could also be a Static method instead of a extension method
• extensions make it hart to see which methods a class has just by looking at it. For example I did not spot AddEnvelopeSignature while looking at the FlowTransaction class.
• ok, this extension method AccountCreatedAddress this one I like. Just a note: .Where(w => w.Type == "flow.AccountCreated").FirstOrDefault() can be written as .FirstOrDefault(w => w.Type == "flow.AccountCreated"). Also if you do FirstOrDefault following it up with a property .FirstOrDefault().Payload will throw an exception if it actually is null. This might be best if it was a custom exception. AccountCreatedAddress also returns a string where ByteString was used earlier for addresses.
• the in-build flow events ("flow.AccountCreated") could be constants
• ...

[tyronbrand]

Thanks for the suggestions @janezpodhostnik, I will get these sorted soon!

[kerrywei]

Thanks for the suggestions @janezpodhostnik, I will get these sorted soon!

Awesome @tyronbrand ! Let us know here when the PR is ready for a second look :)

[tyronbrand]

Hey @janezpodhostnik,

I have made some changes based on your feedback, they are as follows:

• FlowClientAsync.Create("url") is gone and we can now simply use new FlowClientAsync("url").
• Uncommented all examples (except the one that is a known problem on the emulator).
• A FlowAddress class has been added to make it more obvious which fields should be filled with addresses.
• The NewEcdsaAccountKey method has been renamed to GenerateRandomEcdsaKey and a summary added to the method.
• The weight parameter now has a default value of 1000.
• FromHexToByteString handles 0x prefixes.
• Converter classes (ByteStringConverter and HexConverter) have been added to allow for easier testing of conversion methods.
• All conversion methods are now using custom exceptions.
• All methods that use FirstOrDefault() will check if the value returned is null and throw exceptions where required.
• A Cadence base class has been added.
• ICadence has methods that where previously only extension methods.
• Classes such as FlowTransaction and FlowEvent have methods on it that where previously only extension methods.
• Comments have been added/improved on many of the methods commonly used.
• Event constants added.
• Cadence methods have been renamed and look really nice. eg.
  cadenceResponse.As<CadenceComposite>().CompositeFieldAs<CadenceString>("name").Value 😍
• Documentation updated.

NuGet package with these changes is now available.

Great suggestions all around @janezpodhostnik, hopefully I haven't missed any.

[janezpodhostnik]

Very nice! Amazing work.

I have a few comments that could be quickly addressed still:

• Cadence.cs:83 -> if (cadenceValues != null && cadenceValues.Count() > 0) don't use Count > 0 use Any() its much faster, but also in this case there is no need to use either. Just let it go to the foreach with 0 values, that is fine.
• There is no need for both an extension T As<T>(this ICadence cadence) and a class method T As<T>(ICadence cadence) only the class method is sufficient. (same for AddPayloadSignature, AddEnvelopeSignature)
• similar for Decode, but for that one it might be better to stay as a extension method, and be deleted from the class, the reason being that you do not know the type that will be decoded. Another alternative is to have it as a static method on the Cadence class.
• Transaction payer and proposal key and authorisers could also use FlowAddress instead of ByteString

Otherwise the SDK is usable and is relatively nice to use. Overall I would say that with the comments above this would also be good for milestone 4. 🚀 🎉

Here are areas that could use improvements after the flip-fest (and some ideas of nice to have things):

• better test coverage to detect any potential bug, before the users find them :) )
• unifying code style (the code style is inconsistent across the repo)
• error handling (One issue I found is that if you run flow emulator from the wrong folder (thus having the wrong service account key) and then run the examples, the examples fail, but the error makes it really unclear why)
• implementing signing user messages
• cleaner event filtering (getting a specific event from transaction results is a bit messy)
• predefining CadenceComposite types that would automatically get decoded ( for example if I define an CarNFT in cadence I could define a CarComposite in c# and when I would get events or script results that contain a CarNFT it would automatically be of type CarComposite when decoded)

[tyronbrand]

Thanks @janezpodhostnik,

I will make those changes now.

I have added your other improvements as Issues to the repo so they wont be missed.

Would you be able to comment further on this suggestion so I can add it to the Issue's description?

• unifying code style (the code style is inconsistent across the repo)

Really like your final suggestion (predefining CadenceComposite types) 👍

[janezpodhostnik]

Would you be able to comment further on this suggestion so I can add it to the Issue's description?

I noticed some variable names starting with a underscore and my IDE complaining about it, but going back now I either can find it anymore, or more likely that you have already addressed it. I have noticed that jetbrains Rider still complains about other things, but thats best taken with a grain of salt.

[tyronbrand]

Thanks, Il take look.