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(dart/catalyst_cardano_serialization): initial implementation of dynamic coin selection algorithm #1684

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

ilap
Copy link
Contributor

@ilap ilap commented Jan 24, 2025

Description

This PR introduces the initial implementation of a coin selection algorithm in the TransactionBuilder. The algorithm automatically selects the set of unspent transaction outputs (UTXOs) for constructing a transaction.
It aims to minimise transaction fees and improve overall efficiency.

Key changes include:

  • Implemented the dynamic coin selection logic within the TransactionBuilder.

Related Issue(s)

Resolves #1520

Breaking Changes

There are no breaking changes in this pull request.

Screenshots

N/A

Related Pull Requests

N/A

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@dtscalac
Copy link
Contributor

@ilap thank you for the contribution, will review it later today.

@ilap
Copy link
Contributor Author

ilap commented Jan 28, 2025

@ilap thank you for the contribution, will review it later today.

No worries, thx. But, I am going to refactor/simplify the coin selection logic in this week, as the last two commits broke the property-based tests.

@dtscalac
Copy link
Contributor

The spell checker is complaining about these issues:

     +check-spelling *failed* | Repeating the failure error...
     +check-spelling *failed* | --> RUN cspell-cli --quiet lint . --dot
     +check-spelling *failed* | catalyst_voices/packages/libs/catalyst_cardano_serialization/lib/src/builders/input_builder.dart:16:24 - Unknown word (prioritise)
     +check-spelling *failed* | catalyst_voices/packages/libs/catalyst_cardano_serialization/lib/src/builders/input_builder.dart:56:45 - Unknown word (prioritise)
     +check-spelling *failed* | catalyst_voices/packages/libs/catalyst_cardano_serialization/lib/src/builders/transaction_builder.dart:778:38 - Unknown word (lentgh)
     +check-spelling *failed* | catalyst_voices/packages/libs/catalyst_cardano_serialization/lib/src/builders/transaction_builder.dart:844:44 - Unknown word (multiassets)
     +check-spelling *failed* | catalyst_voices/packages/libs/catalyst_cardano_serialization/lib/src/builders/types.dart:31:52 - Unknown word (prioritised)
     +check-spelling *failed* | catalyst_voices/packages/libs/catalyst_cardano_serialization/pubspec.yaml:29:3 - Unknown word (kiri)
     +check-spelling *failed* | catalyst_voices/packages/libs/catalyst_cardano_serialization/test/builders/input_builder_test.dart:2:17 - Unknown word (kiri)
     +check-spelling *failed* | catalyst_voices/packages/libs/catalyst_cardano_serialization/test/builders/input_builder_test.dart:2:28 - Unknown word (kiri)
     +check-spelling *failed* | catalyst_voices/packages/libs/catalyst_cardano_serialization/test/test_utils/selection_utils.dart:376:28 - Unknown word (normalise)
     +check-spelling *failed* | ERROR /var/folders/g1/55kjtgl504v466583q_cg4440000gp/T/earthly-git2873568597/Earthfile:20:4
     +check-spelling *failed* |       The command
     +check-spelling *failed* |           RUN cspell-cli --quiet lint . --dot
     +check-spelling *failed* |       did not complete successfully. Exit code 1

The ones that are not typos should be added to .config/dictionaries/project.dic so that the spell-checker ignores them.

@ilap
Copy link
Contributor Author

ilap commented Jan 28, 2025

Thx, will go through all of them soon.

@dtscalac
Copy link
Contributor

dtscalac commented Jan 29, 2025

@ilap

The changes from this branch are causing the following error in one of the dependent packages (but I think it's an issue with the serializing package):

Screenshot 2025-01-29 at 10 06 09

To reproduce:

  1. cd catalyst_voices/packages/libs/catalyst_cardano/catalyst_cardano/example
  2. flutter run \
    --web-header Cross-Origin-Opener-Policy=same-origin
    --web-header Cross-Origin-Embedder-Policy=require-corp
    --target lib/main.dart
    -d chrome
  3. connect any wallet (test-net preferred)
  4. attempt to sign & submit tx

On current main branch the issue is not reproducible.

@ilap
Copy link
Contributor Author

ilap commented Jan 30, 2025

On current main branch the issue is not reproducible.

Hey @dtscalac, thanks for the heads-up!

My latest commit should fix this. The main issue was that the output fee calculation (feeForOutput) method always returned 0, so I fixed that. I also cleaned up balance handling and tweaked asset mapping.

Can you give it another shot and see if everything works now?

@ilap
Copy link
Contributor Author

ilap commented Jan 30, 2025

Hey @dtscalac , Here are some suggestions:

  • Deprecate buildFake and vkeysCount in WitnessBuilder.
  • Deprecate the following methods in TransactionBuilder:
    • withChangeAddressIfNeeded
    • _withChangeAddressIfNeededWithMultiAssets
    • _withChangeAddressIfNeededWithoutMultiAssets
    • _willAddingAssetMakeOutputOverflow
    • _packNftsForChange

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New
Development

Successfully merging this pull request may close these issues.

[Feature]: Implement coin selection algorithm into TransactionBuilder
3 participants