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

Allow multiple change outputs #1025

Merged
merged 4 commits into from
Jun 16, 2021
Merged

Conversation

PeterBenc
Copy link
Collaborator

Changes:

  • Splits change outputs based on the number of tokens

Gotcha:

  • first commit only moves stuff around, so I recommend to review by commits,
  • Im currently working n tests

@mlenik mlenik temporarily deployed to adalite-allow-multiple--2jya6c June 7, 2021 12:31 Inactive
@PeterBenc PeterBenc temporarily deployed to adalite-allow-multiple--2jya6c June 7, 2021 13:45 Inactive
@PeterBenc PeterBenc force-pushed the allow-multiple-change-outputs branch from 0311bc5 to 409e956 Compare June 7, 2021 13:48
@PeterBenc PeterBenc temporarily deployed to adalite-allow-multiple--2jya6c June 7, 2021 13:48 Inactive
@PeterBenc PeterBenc temporarily deployed to adalite-allow-multiple--2jya6c June 7, 2021 14:35 Inactive
@PeterBenc PeterBenc force-pushed the allow-multiple-change-outputs branch from e47f4df to d95bebe Compare June 7, 2021 14:40
@PeterBenc PeterBenc temporarily deployed to adalite-allow-multiple--2jya6c June 7, 2021 14:40 Inactive
@PeterBenc PeterBenc temporarily deployed to adalite-staging June 7, 2021 17:59 Inactive
@PeterBenc PeterBenc requested a review from refi93 June 8, 2021 07:33
@PeterBenc PeterBenc requested a review from refi93 June 14, 2021 19:03
minimalLovelaceAmount: additionalLovelaceAmount,
}

const outputsWithChange = change ? [...outputs, ...change] : outputs
Copy link
Collaborator

@refi93 refi93 Jun 15, 2021

Choose a reason for hiding this comment

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

empty array is truthy in JS, so I believe this actually always ends up in the left branch, or can the "change" be null? In that case I'd rather be more explicit and check for change != null

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I totally missed this, thanks, will fix 👍

// max token size about 70 bytes, max output size is 4000 => 4000 / 70 ~ 50
export const MAX_OUTPUT_TOKENS = 50

export const MIN_UTXO_VALUE = 1000000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this rather ADA_ONLY_MIN_UTXO_VALUE ?

const totalOutput = outputs.reduce((acc, {coins}) => acc + coins, 0) + deposit
const totalOutputTokens = aggregateTokenBundles(outputs.map(({tokenBundle}) => tokenBundle))

// total amount of lovelace that had to be added to token-containing outputs
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps a single bigger multiline comment at the beginning outlining the steps in human language here would help a lot in figuring out what's going on. The one-line comments scattered around the code help to some extent, but it's already too hard to get a bigger picture out of them - could you write such comment @PeterBenc ?

Copy link
Collaborator

@refi93 refi93 left a comment

Choose a reason for hiding this comment

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

LGTM, I'd just suggest adding a big comment outliningthe steps taken when computing the tx plan

@PeterBenc PeterBenc force-pushed the allow-multiple-change-outputs branch from 4b566b7 to 1cae13c Compare June 16, 2021 08:37
@PeterBenc PeterBenc merged commit 307cbce into develop Jun 16, 2021
@DavidTranDucVL DavidTranDucVL deleted the allow-multiple-change-outputs branch October 15, 2021 15:00
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.

3 participants