feat: transaction template #808

r4mmer · 2024-12-13T17:30:58Z

Acceptance Criteria

Implement the transaction template design (reference)

Code organization

The transaction template code is organized in the following modules:

instructions
- Parsers and definitions of the user input and the formatted data that will be used as transaction templates.
executor
- Methods that will determine how each of the instructions work when running a template.
setvarcommands
- sub-routines to execute with the action/setvar instructions.
interpreter
- Wallet facade interpreter, acts as the bridge between the template and the wallet (fetching utxos, txs, balance.)
context
- State of the template execution and tracking of data (for instance, the balance of the transaction being built).
builder
- A helper class to build a transaction template.
types
- Typing to add abstraction.
- This is required to split the modules in different files, without this we would introduce some import cycles.

Note

The interpreter for the wallet-service is not part of this implementation.

Note

There are some differences between the design and the instructions on the implementation but this is because of a "not-intuitive-behavior" on zod, specially when dealing with unions.

Future possibilities

Code duplication in the executors

In the executors there is some code duplication (especially when creating inputs/outputs and updating the context balance), but I avoided abstracting too much in the initial implementation so we can have more freedom in how we implement each instruction, later I think we should come back to this and make the executor code cleaner.

Choosing change address

An issue in the usual transactions of the wallet facade is that if we get a wallet address to use and then a change address it can be the same address since the wallet does not have the context of other addresses that are being used in the transaction.
The template has this information and it could be used to avoid choosing the same address twice in the same tx.
Since this is not an issue with the wallet facade I did not implement in the tx template but we could easily add this.

codecov · 2024-12-13T17:34:09Z

Codecov Report

Attention: Patch coverage is 81.23953% with 112 lines in your changes missing coverage. Please review.

Project coverage is 80.75%. Comparing base (c06d3ce) to head (33591c3).

Files with missing lines	Patch %	Lines
src/template/transaction/executor.ts	84.36%	53 Missing ⚠️
src/template/transaction/interpreter.ts	63.26%	18 Missing ⚠️
src/template/transaction/builder.ts	75.86%	14 Missing ⚠️
src/utils/transaction.ts	0.00%	14 Missing ⚠️
src/template/transaction/context.ts	87.34%	10 Missing ⚠️
src/template/transaction/setvarcommands.ts	62.50%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #808      +/-   ##
==========================================
+ Coverage   80.68%   80.75%   +0.07%     
==========================================
  Files          85       91       +6     
  Lines        6549     7145     +596     
  Branches     1419     1489      +70     
==========================================
+ Hits         5284     5770     +486     
- Misses       1252     1362     +110     
  Partials       13       13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

r4mmer · 2025-01-02T14:44:51Z

src/new/wallet.js

@@ -2781,7 +2782,7 @@ class HathorWallet extends EventEmitter {
      const tokenIdx = tokenUtils.getTokenIndexFromData(token_data);
      const tokenUid = tokens[tokenIdx - 1]?.uid;
      if (!tokenUid) {
-        throw new Error(`Token ${tokenUid} not found in tokens list`);


if !tokenUid means that tokenUid is undefined or empty string, which makes no sense adding to the error message.
This is why I changed to token_data instead, this makes understanding what went wrong easier.

r4mmer · 2025-01-02T15:25:21Z

src/template/transaction/builder.ts

+    this.template = [];
+  }
+
+  static from(instructions: z.input<typeof TransactionTemplate>): TransactionTemplateBuilder {


Obs: z.input<T> means that this accepts the "unparsed" instruction.
When the type is z.infer<T> it means the output of the parsed data.

r4mmer · 2025-01-02T16:02:37Z

src/template/transaction/instructions.ts

+export const SetVarGetWalletAddressOpts = z.object({
+  method: z.literal('get_wallet_address'),
+  index: z.number().optional(),
+});
+
+export const SetVarGetWalletBalanceOpts = z.object({
+  method: z.literal('get_wallet_balance'),
+  token: TemplateRef.or(TokenSchema.default('00')),
+  authority: z.enum(['mint', 'melt']).optional(),
+});
+
+export const SetVarCallArgs = z.discriminatedUnion('method', [
+  SetVarGetWalletAddressOpts,
+  SetVarGetWalletBalanceOpts,
+]);


This was changed from the design because non discriminated unions with a lot of optionals were being difficult to parse correctly.

Could you explicit what was changed to facilitate a posterior update of the design?

tuliomir · 2025-01-13T14:10:51Z

__tests__/integration/hathorwallet_facade.test.ts

+      .addUtxoSelect({ fill: 1 })
+      .addTokenOutput({ address: '{addr}', amount: 100, useCreatedToken: true })


question: Should we enforce these parameters to be bigints?

Suggested change

.addUtxoSelect({ fill: 1 })

.addTokenOutput({ address: '{addr}', amount: 100, useCreatedToken: true })

.addUtxoSelect({ fill: 1n })

.addTokenOutput({ address: '{addr}', amount: 100n, useCreatedToken: true })

tuliomir · 2025-01-14T00:02:18Z

src/template/transaction/context.ts

+
+  outputs: Output[];
+
+  tokens: string[]; // use token data?


question: What were your ideas for this comment?

At first glance it seems to me that it's better to keep a separate array to manage the context that does not involve the actual Transaction object.

tuliomir · 2025-01-14T00:03:33Z

src/template/transaction/context.ts

+  tokenSymbol?: string;
+
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  vars: Record<string, any>;


suggestion(non-blocking): By using unknown, each call to the vars need to have its type cast, which helps with the type safety and removes the warning.

Suggested change

vars: Record<string, any>;

vars: Record<string, unknown>;

tuliomir · 2025-01-14T00:09:32Z

src/template/transaction/context.ts

+    this.version = CREATE_TOKEN_TX_VERSION;
+  }
+
+  getTokenDataFor(token: string, useCreatedToken: boolean) {


chore: Change the name of the method to better communicate its purpose. As of now, it is a get method that actually mutates the object and adds a new token.

Also, when useCreatedToken is passed, token is discarded, which is also slightly confusing and would be better accompanied by a method docstring to explain it.

tuliomir · 2025-01-14T00:12:42Z

src/template/transaction/context.ts

+    return this.tokens.length;
+  }
+
+  addInput(position: number, ...inputs: Input[]) {


chore: Change the name to better inform that multiple inputs can be added, and that the expected parameter is actually an array and not an object. Initial suggestions would be addInputs or addManyInputs.

tuliomir · 2025-01-15T14:19:48Z

src/template/transaction/executor.ts

+  ctx.log(`amount(${amount})`);
+  if (!amount) {
+    throw new Error('Raw token output missing amount');
+  }


suggestion(non-blocking): Move this validation closer to line 263, so that this quick and mandatory validation can fail early.

tuliomir · 2025-01-15T14:36:29Z

src/template/transaction/executor.ts

+      ctx.addOutput(-1, output);
+    }
+
+    if (balance.mint_authorities > 0) {


thought(non-blocking): It feels weird having change addresses for authorities, since each UTXO can only hold a single authority.

I understand this as a user-friendly way of dealing with the transaction having more authority inputs than needed. Not sure if this should have a comment or any kind of feedback/warning for the user.

tuliomir · 2025-01-15T15:13:10Z

__tests__/integration/template/transaction/template.test.ts

+      .addSetVarAction({ name: 'addr', value: address })
+      .addSetVarAction({ name: 'token', value: tokenUid })


chore: Not related to these lines, but all tests made here use variable values. At least one of them should test passing the address/value directly to the instructions.

tuliomir · 2025-01-15T15:15:07Z

__tests__/integration/template/transaction/template.test.ts

+    expect(tx.hash).toBeDefined();
+  });
+
+  it('should be able to complete change', async () => {


chore: The title is not clear about the difference between this test and the last one.

tuliomir · 2025-01-15T16:11:44Z

__tests__/template/transaction/executor.test.ts

+    ctx.addInput(-1, new Input(txId, Number(i)));
+    arr.push(i);
+  }
+  await executor(interpreter, ctx, ins);


suggestion(non-blocking): Add assertions that ensure the steps are the same with a method like contains, or check the final balance of the transaction to confirm its equivalence to the original one.

r4mmer self-assigned this Dec 13, 2024

r4mmer requested a review from pedroferreira1 as a code owner December 13, 2024 17:30

r4mmer force-pushed the feat/tx-template branch from 5bd68ba to d0db21b Compare December 16, 2024 18:48

r4mmer added 3 commits December 16, 2024 13:49

feat: POC implementation

cbb5490

feat: initial implementation

9755aa8

feat: add support for create token transaction

a342ab7

r4mmer force-pushed the feat/tx-template branch from d0db21b to a342ab7 Compare December 16, 2024 18:50

r4mmer added 19 commits December 17, 2024 09:01

feat: add balance management for created token reference

edb3894

tests: instruction tests

250105d

feat: complete instruction

90dab94

feat: complete instruction

13efa80

feat: executor tests

fec87f6

tests: executor and interpreter tests

fff192b

tests(integration): custom token integration template tests

d46c571

chore: linter changes

6f91f9e

chore: linter changes

3a3b9ea

tests(integration): change injectFunds options

30f1764

tests(integration): create token tx have different serialization

3b2ae37

tests(integration): include change outputs in checks

7099194

tests(integration): change output orderr

edd8234

tests(integration): add change value to send operation

d120896

chore: remove helper types to use inferred types from zod

a50620a

tests(integration): add change and complete tests

ee5edf4

tests(integration): wrong complete instruction type

ab0b78e

fix: change would add invalid tokens on transaction

49ad6d3

tests(unit): mock balance properly

674aa2d

r4mmer requested a review from tuliomir January 2, 2025 14:39

r4mmer commented Jan 2, 2025

View reviewed changes

r4mmer added 7 commits January 7, 2025 14:59

feat: expose transaction template and helper method on the wallet

c7e968c

chore: linter changes

61b9502

test(integration): increase coverage on integration tests

ba4110d

tests(integration): change position of tests

1d8cb3f

tests(integration): change tx version

b2cd834

feat: make tx signing optional when building with the facade

d82aa19

feat: correctly pass the arguments to build template

33591c3

tuliomir reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: transaction template #808

feat: transaction template #808

r4mmer commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 13, 2024 •

edited

Loading

r4mmer Jan 2, 2025

r4mmer Jan 2, 2025

r4mmer Jan 2, 2025

tuliomir Jan 14, 2025

tuliomir Jan 13, 2025

tuliomir Jan 14, 2025

tuliomir Jan 14, 2025

tuliomir Jan 14, 2025

tuliomir Jan 14, 2025

tuliomir Jan 15, 2025

tuliomir Jan 15, 2025

tuliomir Jan 15, 2025

tuliomir Jan 15, 2025 •

edited

Loading

tuliomir Jan 15, 2025

		.addUtxoSelect({ fill: 1 })
		.addTokenOutput({ address: '{addr}', amount: 100, useCreatedToken: true })

		.addSetVarAction({ name: 'addr', value: address })
		.addSetVarAction({ name: 'token', value: tokenUid })

feat: transaction template #808

Are you sure you want to change the base?

feat: transaction template #808

Conversation

r4mmer commented Dec 13, 2024 • edited Loading

Acceptance Criteria

Code organization

Future possibilities

codecov bot commented Dec 13, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuliomir Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r4mmer commented Dec 13, 2024 •

edited

Loading

codecov bot commented Dec 13, 2024 •

edited

Loading

tuliomir Jan 15, 2025 •

edited

Loading