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

refactor: Implement generics in Transaction.go #1089

Merged
merged 16 commits into from
Nov 1, 2024
Merged

Conversation

0xivanov
Copy link
Contributor

@0xivanov 0xivanov commented Oct 2, 2024

Description:

Introduction

The current design of the Transaction struct in Go has 3 major inefficiencies:

  1. Redundant method shadowing: Each transaction type (e.g., AccountCreateTransaction, TokenCreateTransaction, etc.) needs to implement boilerplate functions that simply shadow methods from the base Transaction struct. This results in thousands of lines of redundant code and poor maintainability as the number of methods increases.
  2. Complex static utility functions: Functions such as TransactionExecute and TransactionSign rely on large switch statements to handle various transaction types. As more transaction types are added, the code becomes increasingly complex, leading to maintainability issues and a higher likelihood of errors. For reference, the current lines of code for these static methods is around 4000.
    One such function is this:
  3. Adding new abstractions to handle similar transaction types (Airdrop/Transfer, TokenClaim/TokenCancel) is costly in terms of code duplication and complexity. Every transaction type needs its own implementation, which makes code reuse difficult.

In order to fix all above mentioned problems is by implementing Generics into Transacation.go and transactions.

Overview

Old Architecture

Pros:

  1. Implements the Fluent Pattern:
    • The current SDK design uses the fluent interface, making it easy for developers to chain method calls and maintain a clean, readable API.
      Example: NewTokenCreateTransaction.SetTokenID().SetNodeAccountIDs().Execute()

Cons:

  1. Bad Scaling:
    • The SDK suffers from method shadowing, which leads to thousands of lines of redundant code (currently around 5000 lines). As the number of methods grows, managing these shadowed methods becomes more troublesome, leading to poor scalability.
  2. Costly to Create Intermediate Abstractions:
    • Adding new abstractions to handle similar transaction types is costly in terms of code duplication and complexity. Every transaction type needs its own implementation, which makes code reuse difficult.
  3. Hard to Maintain:
    • The base transaction class is coupled with many concrete subclasses (~100). Introducing new methods in the base class requires updating all the subclasses, creating a maintenance burden.
  4. Prone to Errors - No Compile-time Checks:
    • The current design lacks type safety at compile time, meaning many potential errors can only be caught at runtime. This increases the risk of bugs that are harder to track down.

New Architecture

Pros:

  1. Implements the Fluent Pattern:
    • Like the old architecture, the new architecture retains the fluent interface, ensuring that the API remains developer-friendly for building complex chains of setters.
      Example : NewTokenCreateTransaction.SetTokenID().SetNodeAccountIDs().Execute()
  2. Good Scaling:
    • By introducing a more reusable base transaction, many common transaction types can share the same base logic. This makes the codebase much more scalable and avoids the redundancy issues of the old design.
  3. Easy to Maintain:
    • The base transaction can be extended with additional common fields and methods, making it much easier to modify or enhance functionality without needing to touch every subclass. This significantly reduces the maintenance burden.
  4. Compile-time Checks:
    • The new design leverages Go's type system to enforce stronger compile-time checks, reducing the risk of runtime errors and improving overall code reliability.
  5. Easier to Create Intermediate Abstractions:
    • The new architecture allows more flexibility when creating abstractions. Shared logic can be factored out into reusable components, reducing the amount of duplicate code.

Cons:

  1. Needs one change to the existing api:
    • SignTransaction ’s api needs a minor change. It would need to accept concrete transaction instead of parent transaction.
      Example before: newKey.SignTransaction(&transaction.Transaction)
      Example after: newKey.SignTransaction(transaction)
      • I’ve done some research and this method is not commonly used, but if someone is already consuming this API, it can be changed very easily (only remove the .Transaction at the end).

https://www.notion.so/limechain/Go-SDK-refactor-v2-10610baa8dcd80d7a787d086a3ef3181

Related issue(s):

Fixes #

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
@0xivanov 0xivanov self-assigned this Oct 2, 2024
Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 88.11122% with 124 lines in your changes missing coverage. Please review.

Project coverage is 74.25%. Comparing base (d5cb97f) to head (0874855).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
freeze_transaction.go 0.00% 22 Missing ⚠️
prng_transaction.go 0.00% 19 Missing ⚠️
token_pause_transaction.go 57.89% 8 Missing ⚠️
ethereum_transaction.go 78.94% 4 Missing ⚠️
topic_message_submit_transaction.go 93.33% 3 Missing and 1 partial ⚠️
node_create_transaction.go 84.21% 3 Missing ⚠️
node_update_transaction.go 84.21% 3 Missing ⚠️
token_airdrop_transaction.go 83.33% 3 Missing ⚠️
token_cancel_airdrop_transaction.go 83.33% 3 Missing ⚠️
token_claim_airdrop_transaction.go 83.33% 3 Missing ⚠️
... and 43 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1089      +/-   ##
==========================================
+ Coverage   72.16%   74.25%   +2.09%     
==========================================
  Files         175      174       -1     
  Lines       28770    21624    -7146     
==========================================
- Hits        20762    16057    -4705     
+ Misses       7263     4899    -2364     
+ Partials      745      668      -77     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

crypto.go Show resolved Hide resolved
file_append_transaction.go Show resolved Hide resolved
transaction.go Show resolved Hide resolved
transaction.go Outdated Show resolved Hide resolved
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
@0xivanov 0xivanov marked this pull request as ready for review October 24, 2024 11:53
@0xivanov 0xivanov requested review from a team as code owners October 24, 2024 11:53
Copy link
Contributor Author

@0xivanov 0xivanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not merge until we've release v2.49

@0xivanov 0xivanov changed the title refactor(wip): Implement generics in Transaction.go refactor: Implement generics in Transaction.go Oct 24, 2024
@0xivanov 0xivanov added this to the v2.50.0 milestone Oct 29, 2024
account_update_transaction_e2e_test.go Outdated Show resolved Hide resolved
transaction.go Outdated Show resolved Hide resolved
gsstoykov
gsstoykov previously approved these changes Nov 1, 2024
Copy link
Contributor

@gsstoykov gsstoykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Signed-off-by: Ivan Ivanov <ivanivanov.ii726@gmail.com>
Copy link

sonarqubecloud bot commented Nov 1, 2024

@0xivanov 0xivanov merged commit b2b7688 into main Nov 1, 2024
13 checks passed
@0xivanov 0xivanov deleted the refactor-transaction.go branch November 1, 2024 15:24
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.

2 participants