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

Feature: auto-balance multi-assets #4453

Closed
6 of 15 tasks
dorin100 opened this issue Sep 19, 2022 · 8 comments · Fixed by #4450
Closed
6 of 15 tasks

Feature: auto-balance multi-assets #4453

dorin100 opened this issue Sep 19, 2022 · 8 comments · Fixed by #4450
Assignees
Labels
type: internal feature Non user-facing functionality user type: internal Created by an IOG employee

Comments

@dorin100
Copy link

dorin100 commented Sep 19, 2022

What - The user-facing feature being implemented

Why

  • in order to allow CLI users to have an auto-magic command that will auto-balance all the transaction types

Personas - Who will this affect?

  • SPO
  • Dapp Devs
  • Exchanges

Definition of Done

  • Acceptance Criteria + User Stories & DoD created and singed-off
  • Code & Test review
  • Builds successfully on CI
  • Includes documentation and/or examples for the functionality (+ usage and response examples)
  • Log/record changes on Vnext (or similar depending on what we adopt)
  • Ticket number included in PR description
  • The new functionality is covered by dev/unit/property tests
  • Acceptance Criteria met
  • Test engineer signs-off + system tests fully automated

Sign-off Acceptance Criteria

  • Product Owner
  • Dev Owner
  • Test Engineer Owner

Related PRs

  1. Auto-balance multiasset transactions #4450

Acceptance Criteria

User Story 1

  • As a CLI user (all Personas), when using the transaction build CLI command, I want to auto-balance multi-asset transactions
AC1.1
  • When creating a transaction using the transaction build CLI command, the minFee is paid, and the transaction is auto-balanced when it includes:

    • Only ada
    • ada and only 1 native token
    • ada and various native tokens
    • with max no of multi-asset (based on block size)
    • with multiple inputs and outputs of all possible types (ADA, simple scripts, multi-assets (mint, spend, burn), Plutus scripts, certificates)

User Story 2

  • As a CLI user, I don't expect any of the previously existing functionalities for the transaction build CLI command to be changed
AC2.1
  • all existing automated regression tests should pass

Example of Acceptance test:

  • we need to have tests for all the scenarios from the Acceptance Criteria; preferable all these tests should be covered at unit level

GIVEN a CLI user has 1 UTXO with only 100 ADA, 1 UTXO with 10 dor,
WHEN the user creates a transaction using like below (using both utxos as inputs),
AND sending 9 ADA and 5 dor to a different address
THAN the transaction can be signed without getting any error
AND the transaction can be submitted without getting any error

cardano-cli transaction build \
    --testnet-magic $MAGIC \
    --tx-in UTXO_1 \
    --tx-in UTXO_2 \
    --tx-out DST_ADDR+9000000+5 $policyid.$tokenname"  \
    --change-address CHANGE_ADDR \
    --out-file tx1.body 
@dorin100
Copy link
Author

for the Related PRs we can also use the Development section from the right side.

@CarlosLopezDeLara CarlosLopezDeLara linked a pull request Sep 19, 2022 that will close this issue
@dorin100 dorin100 changed the title Auto-balance multi-assets - Acceptance Criteria & DoD Feature: auto-balance multi-assets Sep 23, 2022
@dorin100
Copy link
Author

@Jimbo4350 is #4446 related to this feature?

@Jimbo4350
Copy link
Contributor

@dorin100 no its not

@Jimbo4350
Copy link
Contributor

Jimbo4350 commented Oct 7, 2022

@LudvikGalois I would create a property test using cardano-testnet and confirm the tx balances in the way we expect for the following cases:

  • A single multiasset (minting 1 and more than one)
  • 2 or more multiassets (minting 1 or more than one)
  • Burning and minting in the same tx with 1 and more than one multiasset
  • The above cases with spending the multiasset(s) as well

Why these cases? Because these are concerned with the feature you are adding i.e the balancing of multiassets. Also having separate cases makes the diagnosis of what is broken way easier.

We should have a separate property test for our tx build command with cardano-testnet that covers:

Only ada
with multiple inputs and outputs of all possible types (ADA, simple scripts, multi-assets, Plutus scripts, certificates)
with max no of multi-asset (based on block size)
but that is another issue.

@dorin100
Copy link
Author

dorin100 commented Oct 7, 2022

@Jimbo4350 there is a good practice of covering all the Acceptance Criteria with automated tests. This is why, our proposal, is first to have a discussion, followed by a sign-off, on the Acceptance Criteria.

@Jimbo4350
Copy link
Contributor

Jimbo4350 commented Oct 7, 2022

Relevant: #4514

@Jimbo4350
Copy link
Contributor

@Jimbo4350 there is a good practice of covering all the Acceptance Criteria with automated tests. This is why, our proposal, is first to have a discussion, followed by a sign-off, on the Acceptance Criteria.

Sure but the acceptance criteria outlined for this feature goes beyond the scope of the feature being introduced. The two cases I outlined above should be enough for this feature. See #4514.

@dorin100 dorin100 added type: internal feature Non user-facing functionality user type: internal Created by an IOG employee labels Oct 21, 2022
@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 120 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: internal feature Non user-facing functionality user type: internal Created by an IOG employee
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants