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

C# library for TON Connect 2.0 #276

Closed
8 tasks done
purpleguy99 opened this issue Jul 14, 2023 · 22 comments
Closed
8 tasks done

C# library for TON Connect 2.0 #276

purpleguy99 opened this issue Jul 14, 2023 · 22 comments
Assignees
Labels
Approved This proposal is approved by the committee Developer Tool Related to tools or utilities used by developers

Comments

@purpleguy99
Copy link
Contributor

purpleguy99 commented Jul 14, 2023

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

  • Develop a library for working with the TON Connect in C#
  • Help C# developers integrate TON Connect into dApps written in C# (using Unity)

Deliverables

  • GitHub repository with the implementation TON Connect protocol in C#
  • GitHub repository with the Unity package that use C# implementation of TON Connect
  • Video tutorial (How use TON Connect in Unity)

Definition of Done

  • GitHub repository with the C# library source code
  • Publishing the C# library to NuGet
  • Create a PR to ton-connect repository with a link to the C# library

  • GitHub repository with the Unity package source code
  • Submit the Unity package to Asset Store
  • Create a PR to ton-connect repository with a link to the Unity package
  • Publishing a video tutorial "How use TON Connect in Unity"
  • C# & Unity library is added to docs.ton.org

Reward

  • Standard TON Footstep NFT
  • 2000 USD in TON equivalent

Oriental Release Date

One month after approving

@purpleguy99 purpleguy99 added the footstep This is a TON Footstep issue label Jul 14, 2023
@delovoyhomie
Copy link
Collaborator

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

@purpleguy99
Copy link
Contributor Author

purpleguy99 commented Jul 15, 2023

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
Copy link

Naltox commented Jul 17, 2023

LGTM!

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

@purpleguy99
Copy link
Contributor Author

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
Copy link
Contributor Author

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: ton-community/ton-docs#340

My wallet: EQAiqHfH96zGIC38oNRs1AWHRyn3rsjT1zOiAYfjQ4NKN64s
@delovoyhomie

@ilyar
Copy link

ilyar commented Sep 6, 2023

@purpleguy99
Copy link
Contributor Author

Ton docs successfully updated. ton-community/ton-docs#340

@purpleguy99
Copy link
Contributor Author

@Naltox any updates?

@delovoyhomie
Copy link
Collaborator

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
Copy link
Contributor Author

@Naltox any updates?

@MiGo826
Copy link

MiGo826 commented Oct 3, 2023

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
Copy link
Contributor Author

purpleguy99 commented Oct 4, 2023

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

@Naltox
Copy link

Naltox commented Oct 6, 2023

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

@delovoyhomie
Copy link
Collaborator

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

@MiGo826
Copy link

MiGo826 commented Oct 9, 2023

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:

  • Add error code handling to all the used HttpClients, for example, in the Send method in the BridgeGateway class.
  • It is worth considering the possibility of reusing HttpClients. You can read about the drawbacks of recreating the HttpClient for each request here.
  • Still, create an ITonConnect interface. This will indeed help developers save some time when writing unit tests.
  • If the code contains a single line with an await call or such a call is made at the end of the method, you can remove the async modifier and return the task object. This will help avoid unnecessary overhead with state machine generation by the compiler. (i.e methods Send and [UnPause] in class BridgeGateway)(https://github.com/continuation-team/TonSdk.NET/blob/dev-connect/TonSDK.Connect/Provider/BridgeGateway.cs#L81)

@purpleguy99
Copy link
Contributor Author

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

@MiGo826
Copy link

MiGo826 commented Oct 15, 2023

@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
Copy link
Contributor Author

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

@delovoyhomie
Copy link
Collaborator

@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 delovoyhomie added Developer Tool Related to tools or utilities used by developers and removed footstep This is a TON Footstep issue labels Oct 18, 2023
@delovoyhomie
Copy link
Collaborator

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
Copy link
Collaborator

@purpleguy99
Copy link
Contributor Author

@delovoyhomie #338

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved This proposal is approved by the committee Developer Tool Related to tools or utilities used by developers
Projects
None yet
Development

No branches or pull requests

5 participants