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

Feat/live 15799 alpaca tron combine #9381

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

sprohaszka-ledger
Copy link
Contributor

@sprohaszka-ledger sprohaszka-ledger commented Feb 27, 2025

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • ...

📝 Description

Add combine function for Alpaca.
Revamp broadcast function to use custom tx+signature serialization, in order to keep the same Alpaca API.
Move some "utils" functions to be closer to the correct subdir.
Add integration tests to "Api" with use of dotenv to keep secret private key. This feature is for local test only, not CI.

❓ Context


🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

Copy link

vercel bot commented Feb 27, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
web-tools ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 28, 2025 3:05pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Feb 28, 2025 3:05pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Feb 28, 2025 3:05pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Feb 28, 2025 3:05pm

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR introduces a new combine function for Alpaca, revamps the broadcast functionality to use a custom transaction and signature serialization, and reorganizes utility functions to a more appropriate subdirectory. In addition, integration tests (using dotenv for local secret management) and updates to coin configuration for Tron (and Tezos) modules are provided.

  • Added the combine function and corresponding unit tests.
  • Updated broadcast implementations to align with the new API behavior.
  • Refactored configuration and utilities import paths across multiple modules.

Reviewed Changes

File Description
libs/coin-modules/coin-tron/src/bridge/utils.test.ts Added tests for txInfoToOperation.
libs/coin-modules/coin-tron/src/api/index.integ.test.ts Integrated tests for combine and broadcast functions using TronWeb.
libs/coin-modules/coin-tron/src/bridge/utils.ts Moved and updated utility functions with a note on duplication.
libs/coin-modules/coin-tron/src/logic/combine.test.ts Added unit tests for the combine function and decodeTransaction routine.
libs/coin-modules/coin-tron/src/api/index.ts Created API export using the new combine, broadcast, and craftTransaction functions.
libs/coin-modules/coin-tron/src/logic/craftTransaction.ts Introduced a stub for the craftTransaction function.
libs/coin-modules/coin-tron/src/logic/broadcast.ts Revamped broadcast functionality; removed explicit error-check on broadcast result.
libs/coin-modules/coin-tron/src/bridge/broadcast.ts Updated broadcast bridge to remove result validation previously handled.
libs/coin-modules/coin-tron/src/config.ts Refactored coin configuration to use buildCoinConfig for consistency.
libs/coin-modules/coin-tezos/src/config.ts Updated to use buildCoinConfig to align with Tron configuration module.
libs/coin-modules/coin-tron/src/bridge/synchronization.ts Adjusted synchronization logic with updated utils imports.
libs/coin-modules/coin-tron/src/bridge/getEstimateFees.ts Changed import paths to use local utils, reflecting a restructuring of code.
libs/coin-modules/coin-tron/src/bridge/index.ts Updated bridge setup to use the new coin configuration setter from tronCoinConfig.
libs/coin-modules/coin-tron/src/bridge/buildOptimisticOperation.ts Altered import path for getOperationTypefromMode to align with the new utils location.
libs/coin-modules/coin-tron/jest.integ.config.js Added dotenv setup to support environment-dependent integration tests.

Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

libs/coin-modules/coin-tron/src/logic/broadcast.ts:16

  • The removal of the error check for the broadcastTron result means that failures might go unnoticed. Consider reintroducing error handling to verify that the broadcast was successful.
await broadcastTron(transaction);

libs/coin-modules/coin-tron/src/api/index.integ.test.ts:71

  • [nitpick] Using non-null assertions on 'signature' may hide potential cases where the signature is missing, potentially causing runtime errors. It is advisable to perform an explicit check before accessing the signature.
signedTrx!.signature![0] as unknown as string,

Comment on lines +16 to 17
await broadcastTron(transaction);

Copy link
Preview

Copilot AI Feb 27, 2025

Choose a reason for hiding this comment

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

Removing the check for a successful broadcast (i.e., verifying the result of broadcastTron) could lead to silent failures. It is recommended to handle error responses appropriately.

Suggested change
await broadcastTron(transaction);
try {
await broadcastTron(transaction);
} catch (error) {
console.error("Failed to broadcast transaction:", error);
throw error;
}

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
Signed-off-by: Stéphane Prohaszka <[email protected]>
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

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.

1 participant