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: add alby support to send bulk #67

Conversation

topether21
Copy link
Collaborator

No description provided.

Copy link

linear bot commented May 14, 2024

@topether21 topether21 marked this pull request as draft May 14, 2024 22:21
@topether21 topether21 force-pushed the ruben/ENG-380/allow-users-to-perform-bulk-transfers-if-they-have-a-cardinal-utxo branch 3 times, most recently from 86b40ee to 1395328 Compare May 14, 2024 22:50
@habibitcoin habibitcoin self-requested a review May 14, 2024 22:54
@topether21 topether21 force-pushed the ruben/ENG-380/allow-users-to-perform-bulk-transfers-if-they-have-a-cardinal-utxo branch from 0e58fa3 to eb89e35 Compare May 15, 2024 06:31
@topether21 topether21 changed the title chore: add alby support to send bulk feat: add alby support to send bulk May 15, 2024
@topether21 topether21 force-pushed the ruben/ENG-380/allow-users-to-perform-bulk-transfers-if-they-have-a-cardinal-utxo branch 2 times, most recently from a711010 to e1fd335 Compare May 15, 2024 21:00
Copy link
Collaborator

@habibitcoin habibitcoin left a comment

Choose a reason for hiding this comment

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

Lookin good! Just want to be able to see the txn before signing

So we show estimated fee first, user hits confirm, then signs stuff, then views the final tx before hitting "send"?

src/app/psbt.ts Outdated Show resolved Hide resolved
src/app/psbt.ts Show resolved Hide resolved
.filter((x) => x.status.confirmed)
.filter((x) => x.value > 10000)
.filter(utxo => !utxo.inscriptionId)
.sort((a, b) => b.value - a.value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe actually better to do smallest to largest

Copy link
Collaborator

Choose a reason for hiding this comment

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

ignore this, nevermind. You are trying to use the largest spendable UTXO as a cardinal if you need to add one

Copy link
Contributor

Choose a reason for hiding this comment

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

we should select and use the largest first

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it uses the biggest first.

src/app/psbt.ts Outdated Show resolved Hide resolved
src/app/psbt.ts Show resolved Hide resolved
@topether21 topether21 force-pushed the ruben/ENG-380/allow-users-to-perform-bulk-transfers-if-they-have-a-cardinal-utxo branch from e1fd335 to f5f3f14 Compare May 15, 2024 23:02
@topether21 topether21 force-pushed the ruben/ENG-380/allow-users-to-perform-bulk-transfers-if-they-have-a-cardinal-utxo branch from 78f8255 to 5d32717 Compare May 16, 2024 22:58
@topether21 topether21 requested a review from habibitcoin May 16, 2024 23:09
@topether21 topether21 marked this pull request as ready for review May 16, 2024 23:09
Copy link
Collaborator

@habibitcoin habibitcoin left a comment

Choose a reason for hiding this comment

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

Amazingggg

@@ -331,6 +338,183 @@ const Psbt = function (config) {
return psbtModule.broadcastPsbt(psbt);
},

preparePsbtForMultipleSend: async ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe add comment to describe general flow:

preparePsbtForMultipleSend allows safe bulk UTXO transfers, with appropriate fees.
1. Any input UTXOs with an inscription will be created as a distinct output UTXO of the same size, and appear before any non-inscription UTXOs in the resultant PSBT
2. Any input UTXOs without an inscription will have their amounts consolidated into a single output UTXO
3. A check is run to ensure that enough cardinal funds are available to perform the transfer. If step 2 does not contain enough cardinal funds, cardinal funds will be appended to the PSBT, and another change output will be created

throw new Error('Signing not supported.');
}

const cardinalUtxos = ownedUtxos.filter(utxo => !selectedUtxos.some(ownedUtxo => ownedUtxo.txid === utxo.txid))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe add comment

// only used if selectedCardinalAmount is not large enough to cover fees

}
inputs.push(utxo);
totalCardinalAmount += utxo.value;
isCardinalAdded = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

love it

address,
value: changeAmount,
});
metadata.outputs.push({ type: 'Change' });
Copy link
Collaborator

Choose a reason for hiding this comment

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

excellent

@topether21 topether21 force-pushed the ruben/ENG-380/allow-users-to-perform-bulk-transfers-if-they-have-a-cardinal-utxo branch from a6a1347 to 463167e Compare May 17, 2024 01:36
Copy link
Collaborator

@habibitcoin habibitcoin left a comment

Choose a reason for hiding this comment

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

nice optimization

@habibitcoin habibitcoin merged commit 9a79b25 into main May 18, 2024
1 check failed
@habibitcoin habibitcoin deleted the ruben/ENG-380/allow-users-to-perform-bulk-transfers-if-they-have-a-cardinal-utxo branch May 18, 2024 01:27
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.

4 participants