[BTC] Tx crafting by regnarock · Pull Request #60 · AdamikHQ/adamik-api

regnarock · 2024-07-30T10:27:29Z

This pull request adds support for:

transaction crafting:
- with basic UTXO selection using bitcoinjs coinselect.
  - ⚠️ Known issues:
    - Because the lib is only published in NPM without its types (because the maintainer doesnt trust NPM), I've patched the lib using their own definitions.
    - The algorithm to select UTXOs is pretty rough though. i.e: it always takes the biggest available UTXO of the account, segregated witness support doesn't save as much fees as it should, segwit seems to not be fully implemented, taproot is just not supported at all... etc
- extremely basic, and frankly a little bad, change address. We just pick one of the addresses in the available inputs. It saves a network call and any calculations or cache.
- RBF support according to this "doc"

Incoming:

Support for useMaxAmount field in the API transaction intent.

* Align all cosmos packages versions * Update config: celestia nodeURL & dydx minGasPrice * Improve logs for all Cosmos node error messages * Better handle pubKey and other cosmos account data * Update dYdX node + minor fixes * Change nonce type from number to bigint + actually include it during cosmos completeTransaction

This reverts commit 132abf0.

This reverts commit 294e452.

This reverts commit e9632f8.

vercel · 2024-07-30T10:27:31Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
adamik	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 15, 2024 1:24pm

h-adamik · 2024-08-01T11:00:55Z

+  },
+  "pnpm": {
+    "patchedDependencies": {
+      "coinselect@3.1.13": "patches/coinselect@3.1.13.patch"


Do you have some context about the need for this patch? A link or anything.
Not mandatory just in case, could be useful for future reference.

I put those links as context in the description of the PR:

with basic UTXO selection using bitcoinjs coinselect.
⚠️ Known issues:
Because the lib is only published in NPM without its types (bitcoinjs/coinselect#77 (comment)), I've patched the lib using their own definitions.

But your comment makes me think that I should put that context closer to the codebase itself. A recommendation?

I chose to add the following documentation right next to the patch.

h-adamik · 2024-08-01T11:04:26Z

+      /**
+       * add fields for compatibility with coinselect @see {@link UTXO}
+       */


(nitpicking)
Single-line comment when possible, for more compact code

I just wanted to have the @see {@link UTXO} picked up as JsDoc. And VS code only parses them when between /** .... */ 🙈

Realised it could be done in one line 🤦

h-adamik · 2024-08-01T11:06:04Z

+   * This should never happen because the transaction is already validated by zod
+   * @see {@link transactionDataSchema.refine} at the {@link transactionDataSchema} definition
+   * TODO: maybe we could make sure that completeTransaction directly receives a multi-recipient transaction ?
+   */
+  if (!isMultiRecipientTransaction(transaction.data)) {
+    throw new Error(`Something went wrong, the transaction is not a multi-recipient transaction.`);
+  }


Should be solved after merging MultiRecipientTransaction into the regular TransferTransaction, if I followed correctly 🙂

h-adamik · 2024-08-01T11:06:58Z

-  transaction: TransactionWrapper
-): Promise<void> {
-  throw new Error("Not implemented");
+async function completeTransaction(this: BtcService, transaction: TransactionWrapper): Promise<void> {


Just noticing useMaxAmount is not handled yet. Is it planned? (not a blocker, Fabrice said many times that this bonus feature would drop if it was any trouble)

Just added it! ✔️

h-adamik · 2024-08-01T11:14:42Z

  mode: TransactionMode.TRANSFER_TO_MANY;
  sender: Xpub;
  recipients: Recipient[];
+  inputs?: UtxoWithMetadata[];


That's new though! Would need to figure out if it makes sense in a regular TransferTransaction 🤔

However I don't remember if we discussed it, and sorry if we didn't, but this raises the question: should the client be given the possibility to pick UTXOs?

I am tempted to say 'no', at least for now. The Adamik API should only handle a single amount, both input and output, and manage UTXOs completely internally and behind the scenes from the user.
Even if we do in the future expose the inputs/outputs to the user, this will have to be an additional, bonus feature, on top of the standard single amount. This is important so Bitcoin can still be handled the same as the other chains on client side.

That's just the idea, but do you see any issue proceeding that way?

Excellent points!

I had discussed the introduction of inputs as one of the reasons for a different tx model. Although, those are just for the user's information, they're not use as a input field (paradoxically 🤣) by the API.
So we could:

remove it altogether (and only use an internal model to transit the input data between BTC family Service implementations) => my favourite (but longer to do)

keep it as optional for debug purposes on the client side. => my actual recommendation (it's not perfect, but allows for a quick integration of BTC within the API and we could always remove this field later)

Yes we discussed it and decided that the client should not be able to (for now).

=> I'll go for the introduction of an internal model to forward the input data between functions of the BTC service.

😮 I just realised we're already talking about the inner model, the schema doesn't expose the inputs.

So it's only an internal model problem:
-> knowing we already agreed on having a regular TransferTransaction having family dependents optional fields (like gas)
=> I wouldn't see an issue in having an inputs for BTC usage only in here too.

h-adamik · 2024-08-01T11:23:04Z

+
+  // Enrich inputs from the explorer
+  await Promise.all(
+    transaction.data.inputs.map(async (input, inputIndex) => {


Maybe I'm missing something, but aren't you always adding all inputs to the psbt?

Right I am. At minimum we always have 2 recipients:

the actual recipient of the transaction intent

the change recipient

h-adamik · 2024-08-01T11:24:14Z

+    })
+  );
+
+  for (const recipient of transaction.data.recipients) {


About recipients, don't you need to add the change output manually? And effectively add a recipient to the input transaction?

If I get it correctly, here you handle every recipient that's in the input transaction, i.e for an actual transfer request to multiple addresses (which by the way doesn't need to be supported, nice-to-have feature if we can have it at no cost)

The change output was indeed added manually during the completeTransaction step: https://github.com/AdamikHQ/adamik/pull/60/files#diff-919690c591b9a53acc95310ea442b358e27fb74d4241dc1e1b20423e3d01323cR55

In all the inputs are already setup when we enter encodeTransactionToSign.
I have to handle multiple inputs because it's actually the default case for a BTC tx.

* Complete Cosmos staking implementation * Update OpenAPI JSON docs * Add prevalidation that transaction mode is supported for the chain * Update OpenAPI JSON docs * Better handle validation of token balance --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Additionally: - improved coinselect types and documentation - moved validation from zod to BTC service prevalidate as handling of simple transactions is widely different in BTC - fixed a potential race condition in transaction encoding

regnarock · 2024-08-13T18:43:58Z

Should be ready to merge now as soon as 🟢 for you @hakim-adamik 👍

# Conflicts: # pnpm-lock.yaml # public/openapi.json # src/app/api/transaction/broadcast/schema.ts # src/app/families/common.ts # src/app/model/types_WIP.ts

# Conflicts: # package.json # pnpm-lock.yaml # public/openapi.json # src/app/api/transaction/broadcast/schema.ts # src/app/families/common.ts # src/app/model/types_WIP.ts

# Conflicts: # src/app/config/chains.ts # src/app/families/bitcoin/backend/explorer.ts # src/app/families/bitcoin/backend/types.ts # src/app/families/bitcoin/service/getAddressesState.ts # src/app/model/types_WIP.ts

github-actions · 2024-08-15T13:23:27Z

OpenAPI Comparison results: 1 changes: 1 error, 0 warning, 0 info

regnarock · 2024-08-15T18:45:07Z

Damn! Github misinterpreted a faulty merge on btc_balances and automatically closed this PR and removed the branch 🤦

Reopened here just for the merge back to btc_balances:

h-adamik and others added 11 commits July 20, 2024 12:41

Update Providers Doc

6c1781d

EVM family: switch default transaction format from json to hex

ac24a9d

DEBUG LOG, TO BE REVERTED

294e452

DEBUG LOG, TO BE REVERTED

132abf0

Revert "DEBUG LOG, TO BE REVERTED"

e19dc5b

This reverts commit 132abf0.

Revert "DEBUG LOG, TO BE REVERTED"

9b6ba0d

This reverts commit 294e452.

Change dYdX node

604312b

Update Providers Doc

4db7c3f

Revert "fix: remove useless type guard of multiRecipientTx

260198c

This reverts commit e9632f8.

feat: Adds simple TX crafting

82885d1

vercel Bot deployed to Preview July 30, 2024 10:28 View deployment

h-adamik reviewed Aug 1, 2024

View reviewed changes

h-adamik and others added 4 commits August 1, 2024 17:01

R.I.P original test-app

5cbb6bf

[ADV-280] ADV fix parity (#62)

423f4a8

chore: code quality

b7d714c

vercel Bot deployed to Preview August 12, 2024 17:59 View deployment

feat: adds support for useMaxAmount

6819ac4

Additionally: - improved coinselect types and documentation - moved validation from zod to BTC service prevalidate as handling of simple transactions is widely different in BTC - fixed a potential race condition in transaction encoding

vercel Bot had a problem deploying to Preview August 13, 2024 18:40 Failure

fix: patch package.lock version

0f8f760

regnarock changed the title ~~[WIP][BTC] Tx crafting~~ [BTC] Tx crafting Aug 13, 2024

vercel Bot deployed to Preview August 13, 2024 18:43 View deployment

regnarock added 2 commits August 14, 2024 17:58

Merge branch 'main' into feat/btc_data_and_api

e66443c

# Conflicts: # pnpm-lock.yaml # public/openapi.json # src/app/api/transaction/broadcast/schema.ts # src/app/families/common.ts # src/app/model/types_WIP.ts

Merge branch 'feat/btc_data_and_api' into btc_tx_crafting

334ff60

# Conflicts: # package.json # pnpm-lock.yaml # public/openapi.json # src/app/api/transaction/broadcast/schema.ts # src/app/families/common.ts # src/app/model/types_WIP.ts

vercel Bot deployed to Preview August 14, 2024 22:12 View deployment

Merge branch 'feat/btc_balances' into btc_tx_crafting

1b128db

# Conflicts: # src/app/config/chains.ts # src/app/families/bitcoin/backend/explorer.ts # src/app/families/bitcoin/backend/types.ts # src/app/families/bitcoin/service/getAddressesState.ts # src/app/model/types_WIP.ts

Update OpenAPI JSON docs

b00e97e

vercel Bot deployed to Preview August 15, 2024 13:24 View deployment

regnarock merged commit e058370 into feat/btc_balances Aug 15, 2024

regnarock deleted the btc_tx_crafting branch August 15, 2024 18:21

regnarock mentioned this pull request Aug 15, 2024

[BTC] Tx crafting #65

Merged

1 task

Conversation

regnarock commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel Bot commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

regnarock commented Aug 13, 2024

Uh oh!

github-actions Bot commented Aug 15, 2024

Uh oh!

regnarock commented Aug 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

regnarock commented Jul 30, 2024 •

edited

Loading

vercel Bot commented Jul 30, 2024 •

edited

Loading