Feat/coin control ordering

[komret]

Description

I tried to bear performance in mind.

Related Issue

Resolve #14545

Screenshots:

[github-actions]

🚀 Expo preview is ready!

• Project → trezor-suite-preview
• Platforms → android, ios
• Scheme → trezorsuitelite
• Runtime Version → 21
• More info

Learn more about 𝝠 Expo Github Action

[peter-sanderson]

I suggest to get rid of the let by doing something like this:

const sortFromNewestToOldest: UtxoSortingFunction =
    ({ accountTransactions }) =>
    (a: AccountUtxo, b: AccountUtxo) => {
        if (a.blockHeight > 0 && b.blockHeight > 0) {
            return b.blockHeight - a.blockHeight;
        }

        // Pending transactions do not have blockHeight, so we must use blockTime of the transaction instead.
        const getBlockTime = (txid: string) => {
            const transaction = accountTransactions.find(transaction => transaction.txid === txid);

            return transaction?.blockTime ?? 0;
        };

        return new BigNumber(getBlockTime(b.txid)).comparedTo(new BigNumber(getBlockTime(a.txid)));
    };

[komret]

I kept the lets there because it just seems easier with the secondary sorting.

[peter-sanderson]

Please reconsider if you decide to refactor secondary sorting by wrapping it from outside.

[komret]

82fc888

[peter-sanderson]

I have tested it and it works. Good job!

However I would like to see tests for the sorting function especially the complicated on (largest/newest) + some code polishing.

[komret]

However I would like to see tests for the sorting function especially the complicated on (largest/newest) + some code polishing.

Thanks, I shamelessly stole your commits. Apologies for not using fixups :( I only made changes to the third commit, though.

The changes I made:

• move sorting functions to a separate file
• implement secondary and tertiary sorting by txid and vout
• add unit tests

[peter-sanderson]

The test description should not start with should. I believe we even have it in Notion somewhere (but I am lazy to search it).

Suggested change

	it('should sort UTXOs by newest first', () => {
	it('sorts UTXOs by newest first', () => {

[peter-sanderson]

The (unit) test shall test only one thing. And the assertion shall be done in a way, it is clearly visible what is important (the amount of transaction). This way to work with test I have to go back and forth to see what hides behind UTXOS[1]

This is what I believe is much more readable, and maintainable:

    it('sorts by size, largest first', () => {
        const utxos = [
            testMocks.getUtxo({ amount: '1', txid: 'txid1' }),
            testMocks.getUtxo({ amount: '2', txid: 'txid2' }),
        ];
        const sortedUtxos = sortUtxos(utxos, 'largestFirst', ACCOUNT_TRANSACTIONS);
        expect(sortedUtxos.map(it => it.amount)).toEqual(['2', '1']);
    });

[komret]

Ok I like it, but I think this is more beneficial in the expect clause than the input where some lines can be saved: 82fc888

[peter-sanderson]

In general I believe that being explicit in tests and make it clear what goes in and out is more important then saving some lines.

For example: UTXOS.slice(1, 4) gives me no insight what is the given state of the test.

I think all tests shall follow the pattern:

• given (input of the test)
• when (what is tested)
• then (assert of the output)

All technical details can be hidden and abstracted, but I think the input (what is tested case), the test subject (what piece is tested), and result (what is expected result) shall be visible on the very first sight.

---

To saving some lines maybe this can be shorted:

        const utxos = [
            testMocks.getUtxo({ amount: '2', txid: 'txid2' }),
            testMocks.getUtxo({ amount: '2', txid: 'txid1', vout: 1 }),
            testMocks.getUtxo({ amount: '2', txid: 'txid1', vout: 0 }),
        ];

to something like:

const utxos = [utxo_amount2_tx2_vout1, utxo_amount2_tx1_vout1, , utxo_amount2_tx1_vout0]

But I think its marginal.

---

Anyway, this is not the hill to die on, but I just wanted to explain my reasoning.

[komret]

Thanks, your explanation makes sense. I'd prefer not to change it here now that it is done, but I'll bear it in mind next time.

[peter-sanderson]

here is another suggestion aa0c473

It is a draft, feel free to use it, or not. Up to you.

Espetially tests for newestFirst and oldestFirst are bit messy. But I wanted to show the principles.

[komret]

@peter-sanderson please check 82fc888. If you approve, I will squash it all to the third commit.

[peter-sanderson]

Suggested change

	it('should return the original array if utxoSorting is undefined', () => {
	it('returns the original array if utxoSorting is undefined', () => {

[komret]

Fixed.

[peter-sanderson]

👍 GJ! 🚀