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

added accumulateProof fn #51

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

Conversation

Egge21M
Copy link
Collaborator

@Egge21M Egge21M commented Apr 20, 2023

Added a utility function that will iterate over an array of proofs and gather them until a certain amount is reached. The function will take a strategy argument, which determines in which order proofs are selected:

  • middle: Will alternate between lowest and highest amount
  • ascending: Will start with the lowest amount and work its way up
  • descending: Will start with the highest amount and work its way down.

The function returns an object with three properties:

  • base: An array that contains all proofs but the last one (Proof[)]
  • exceeds: The last Proof that was added to the result (which will make the total amount of gathered proofs greater than the required amount (Proof)
  • excess: The amount that the total amount of gathered proofs exceeds the required amount. (number)

Copy link
Contributor

@BilligsterUser BilligsterUser left a comment

Choose a reason for hiding this comment

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

could you pls run

npm run lint

and

npm run format

@@ -95,11 +95,63 @@ export function checkResponseError(err: unknown) {
}
}
}

function accumulateProofs(
proofs: Proof[],
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use the Array<> notation, for consistency. 😅

case 'middle': {
while (temp.length && total < requiredAmount) {
const first = temp.shift();
total += first!.amount;
Copy link
Contributor

Choose a reason for hiding this comment

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

just to make the linter (and me) happy

case 'middle':
			while (total < requiredAmount) {
				const first = temp.shift();
				if (!first) { break }
				total += first.amount;
				result.push(first);
				if (total >= requiredAmount) {
					break;
				}
				const last = temp.pop();
				if (!last) { break }
				total += last.amount;
				result.push(last);
			}
			break;

		

strategy: 'middle' | 'ascending' | 'descending'
) {
const result: Proof[] = [];
const temp = proofs.slice().sort((a, b) => a.amount - b.amount);
Copy link
Contributor

@BilligsterUser BilligsterUser Apr 21, 2023

Choose a reason for hiding this comment

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

why not

const temp = proofs.slice()
	if (strategy === 'descending') {
		temp.sort((a, b) => b.amount - a.amount);
	} else {
		temp.sort((a, b) => a.amount - b.amount);
	}

and than

case 'descending':
case 'ascending':
	for (let i = 0; i < temp.length; i++) {
		total += temp[i].amount;
		result.push(temp[i]);
		if (total >= requiredAmount) {
			break;
		}
	}
	break;

@Egge21M
Copy link
Collaborator Author

Egge21M commented Apr 21, 2023

could you pls run

npm run lint

eslint yells at me for using !... The reason for non-null assertion in this case is that shift is typed to return a value or undefined if the array is empty. In my case I know that the array will not be empty, because the loop will only run if temp.length is truthy... Unfortunately the TS does not catch this and will complain about first possibly being undefined...

@gandlafbtc
Copy link
Collaborator

We can expose this function on index.ts. maybe at some point it will make sense to expose an util object or something, it's becoming more and more helper methods. Starts to get confusing

@gandlafbtc
Copy link
Collaborator

@BilligsterUser @Egge7 what shall we do with this PR? Has everything been resolved?

@BilligsterUser
Copy link
Contributor

I´m still waiting for @Egge7

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.

None yet

3 participants