ton-society / grants-and-bounties

TON Foundation invites talent to imagine and realize projects that have the potential to integrate with the daily lives of users.
https://ton.org/grants
300 stars 134 forks source link

C# library for TON Connect 2.0 #276

Closed purpleguy99 closed 11 months ago

purpleguy99 commented 1 year ago

Summary

Our goal is to develop a C# library for interaction with the TON Connect 2.0 protocol.

Context

There is currently no available library to support TON Connect protocol implementation within the .NET ecosystem. Given the versatility of the multi-platform .NET framework, this proposed C# library will expand opportunities for application development across various environments. This is particularly noteworthy for developers utilizing the widely used Unity platform.

Goals

Deliverables

Definition of Done

Reward

Oriental Release Date

One month after approving

delovoyhomie commented 1 year ago

@purpleguy99, we allocate about $1,000 for such TC 2.0 integrations. Will it convenient for you?

purpleguy99 commented 1 year ago

2.0 integrations.

Hello! In addition to the C# library, there are also plans to implement the module in Unity. There will also be a tutorial on how to connect and work with Ton Connect in Unity.

Naltox commented 1 year ago

LGTM!

One thing i want to change: let's add "C# & Unity library is added to docs.ton.org" to definition of done

purpleguy99 commented 1 year ago

LGTM!

One thing i want to change: let's add "C# & Unity library is added to docs.ton.org" to definition of done

Done!

purpleguy99 commented 1 year ago

Done! I have released v0.1.0 of the TonConnect SDK for C# and Unity.

C# SDK repository: https://github.com/continuation-team/TonSdk.NET/tree/main/TonSDK.Connect Unity asset repository: https://github.com/continuation-team/unity-ton-connect

Nuget Package: https://www.nuget.org/packages/TonSdk.Connect/ Unity Asset Package: https://github.com/continuation-team/unity-ton-connect/releases/tag/v0.1.1-alpha (now its available in github repository in topic "Releases". Thats cause its takes time to approve asset package in Unity Store.) image image

PR to ton docs: https://github.com/ton-community/ton-docs/pull/340

My wallet: EQAiqHfH96zGIC38oNRs1AWHRyn3rsjT1zOiAYfjQ4NKN64s @delovoyhomie

ilyar commented 1 year ago

https://twitter.com/EverUniver/status/1699504849480552820

purpleguy99 commented 1 year ago

Ton docs successfully updated. https://github.com/ton-community/ton-docs/pull/340

purpleguy99 commented 1 year ago

@Naltox any updates?

delovoyhomie commented 1 year ago

As far as I know, @Naltox has just found a person who is reviewing this bounty right now.

@Naltox, can you please give an approximate completion date?

purpleguy99 commented 1 year ago

@Naltox any updates?

MiGo826 commented 1 year ago

Hi, @purpleguy99! I was asked to review your solution for compliance with the DoD and Deliverables, and check overall implementation quality as well.

When checking for compliance with the requirements, an assessment will be given of how closely the source code aligns with the workflows described on the link page, with the existing sdk written in TypeScript serving as a reference.

The assessment of implementation quality is made from the perspective of a potential library user. This means that the evaluation is primarily focused on the public interface and the logic of the library's operation. The Microsoft guidelines for implementing libraries and frameworks for the .NET platform will be used to evaluate the implementation quality.

Functionality Assessment

Based on the comparison of components and their parts in TonSDK.NET and TS sdk, it can be assumed that the .NET SDK implements the workflow to the required extent. However, to fully assess this, integration with the existing APIs needs to be tested using a sample application.

Implementation Quality Assessment

Critical Observations

ConfigureAwait

The code extensively uses TPL (Task Parallel Library), but considering that the SDK should be a general-purpose library, the method ConfigureAwait(false) is not used anywhere when calling asynchronous methods. This can potentially lead to performance loss when using the library in environments with a SynchronizationContext (e.g., Avalonia), as any code after an await will execute in the thread that invoked the asynchronous operation, even though it could be executed in any thread.

https://devblogs.microsoft.com/dotnet/configureawait-faq/

Multiple exceptions with the same error message

In the [Exceptions.cs](https://github.com/continuation-team/TonSdk.NET/blob/main/TonSDK.Connect/Exceptions.cs) file, all exceptions inheriting from TonConnectError try to override the Info field to set the error message. The problem is that they do this using the new modifier, which means that when an exception is thrown, the message defined in the base class will not use the overridden Info field. In other words, all exceptions will be displayed with the same error message. https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/new-modifier

SSEClient

https://github.com/continuation-team/TonSdk.NET/blob/main/TonSDK.Connect/Provider/SSEClient.cs

The SSEClient class takes a ListenEventsFunction delegate in its constructor. This delegate is not mandatory, and if it is not provided, the client will use the default logic, which includes creating an HttpClient and making a call to the event source. The problem here is that the configured means for calling the event source is not passed to the aforementioned delegate. In other words, library users need to figure out on their own that this code should handle establishing the connection and processing the responses. Furthermore, it should pass the results of processing these responses to other delegates, based on the logic.

All delegates are initialized in a separate model that should be passed to the constructor of the root TonConnect object. However, these capabilities are not mentioned in the documentation (except for a single scenario, the documentation does not describe anything at all).

Furthermore, if a user-defined handler decides to rethrow an exception or throws its own exception, it will result in the application terminating critically because the ListenForEvents method is defined as async void. Ideally, this code should return a Task that can be worked with by the calling code.

Less Critical Observations

Console.Write for logging

All logging is done using the Console.Write methods. This approach does not allow users to integrate library logs with the logging system in their projects or configure log levels.

Lists of delegates as event handlers

C# has a built-in mechanism for the safe implementation of this pattern - Events. The most disappointing aspect here is that these delegates are implicitly passed throughout the code between different parts of the library.

Inconsistent use of nullable reference types

The [connect](https://github.com/continuation-team/TonSdk.NET/blob/main/TonSDK.Connect/TonConnect.cs#L122) method can return null, but the library user cannot know this from the method signature. However, the method signature includes the connectAdditionalRequest parameter with a nullable reference type.

Lack of Encapsulation

All components of the library are declared as public, which allows the application using the library to see and use the internals of the library. Over time, this can lead to significant maintenance complexity and breaking changes with each new release.

Minor Observations

Absence of an interface for the TonConnect class

This doesn't allow integration code to substitute the implementation for testing purposes.

Lack of tests for the TonSdk.Connect library

Additionally, there is a sparse test coverage for everything else.

Final thoughts

It is evident that a lot of work has been done to implement the library. To meet the users' needs for a high-quality C# SDK implementation, it is recommended to address the above-mentioned critical issues and consider aligning the library with Microsoft's guidelines for developing libraries for the .NET platform. As an example to follow, I can suggest the following libraries: https://github.com/jbogard/MediatR https://github.com/npgsql/npgsql/tree/main

purpleguy99 commented 1 year ago

Hello @MiGo826 . Thanks for reviewing our solution. Based on this comments, we decided to create a separate branch to correct the critical observations and correct some less important points.

I would like to draw your attention to the fact that some comments are really important, and we immediately decided to correct them. In addition, the TonSDK.Net library is not only used in Dotnet projects and applications, but must also provide compatibility with the Unity game engine. I would like to inform you that Unity has its own approaches to handling network requests. When developing the library, we primarily focused on compatibility, and therefore there was a need to create delegates to override some methods such as ListenEvents, etc.

Finally, as part of this "Ton Footstep", we tried to fix and organize the code based on your comments. We will release some non-critical fixes as our library evolves, as they are planned to affect the entire TonSDK.NET, not just the .Connect namespace.

Please review the footstep task and leave the feedback. Other observations will be fixed as our library evolves.

I'm attaching a separate branch with the changes. https://github.com/continuation-team/TonSdk.NET/pull/26

Naltox commented 1 year ago

@MiGo826 thanks for the review! Great work 🔥 @delovoyhomie i think we should reward @MiGo826 for his work

delovoyhomie commented 1 year ago

@MiGo826, we appreciate your hard work. How much do you want to receive as a reward as compensation?

MiGo826 commented 1 year ago

Hello @MiGo826 . Thanks for reviewing our solution. Based on this comments, we decided to create a separate branch to correct the critical observations and correct some less important points.

I would like to draw your attention to the fact that some comments are really important, and we immediately decided to correct them. In addition, the TonSDK.Net library is not only used in Dotnet projects and applications, but must also provide compatibility with the Unity game engine. I would like to inform you that Unity has its own approaches to handling network requests. When developing the library, we primarily focused on compatibility, and therefore there was a need to create delegates to override some methods such as ListenEvents, etc.

Finally, as part of this "Ton Footstep", we tried to fix and organize the code based on your comments. We will release some non-critical fixes as our library evolves, as they are planned to affect the entire TonSDK.NET, not just the .Connect namespace.

Please review the footstep task and leave the feedback. Other observations will be fixed as our library evolves.

I'm attaching a separate branch with the changes. continuation-team/TonSdk.NET#26

@purpleguy99, I see that many problems have been successfully resolved. Good job! I have a few suggestions for further improving the library:

purpleguy99 commented 1 year ago

@delovoyhomie Task is done. When will i receive the reward?

MiGo826 commented 1 year ago

@MiGo826, we appreciate your hard work. How much do you want to receive as a reward as compensation?

I apologize for the delayed response. The entire review took approximately 4-5 hours, including the examination of the source materials and the preparation of a document with comments. I believe a sum of $50 will cover my efforts, or whatever amount you deem appropriate.

My wallet: UQB_gv1WV_mhw2ItwMfsxhBviO2dJtkjm271wgsRR8YZK8jf

purpleguy99 commented 1 year ago

@Naltox @delovoyhomie When will i receive the reward? it's all so long

delovoyhomie commented 1 year ago

@purpleguy99, you don't need to do anything else, the review has been successfully completed, the payment request has already been created.

We appreciate your time, but please note that we also expected to do the work from you! Thank you!

delovoyhomie commented 11 months ago

To accurately recognize your valuable contributions in our repository, we kindly request you to create a Pull Request in the Hall of Fame file with your information and Bounty Task details.

Please follow these steps: 1) Fork the repository (if you haven't already). 2) Edit the Hall of Fame file, commit, and push your changes. 3) Create a Pull Request from your fork to the main repository.

For reference on what your entry should look like, please see the examples of past merged pull requests.

@purpleguy99, thank you for your contribution!

delovoyhomie commented 11 months ago

Rewards sent!

Questbook proposal

purpleguy99 commented 11 months ago

@delovoyhomie https://github.com/ton-society/grants-and-bounties/pull/338