(small breaking change) Merge v13's "TransactionNext" into the existing "Transaction" class #396

andreibancioiu · 2024-03-11T10:04:13Z

The existing Transaction class has been adjusted to match the specs.

TransactionNext of v13 has been incorporated into Transaction.

Breaking (but small) change on `tx.version` and `tx.options`

Before (v12):

// @deprecated
version: TransactionVersion;

// @deprecated
options: TransactionOptions;

Now (v13):

version: number;
options: number;

However, in practice, dApps should not be affected by this. The fields version and options were marked as deprecated for a long time. This change does NOT affect the following (since they rely on the not-changed setVersion and setOptions):

mx-sdk-js-hw-provider
sdk-dapp

There, version & options are mutated like this (example):

transaction.setVersion(TransactionVersion.withTxOptions());
transaction.setVersion(TRANSACTION_VERSION_WITH_OPTIONS);
transaction.setOptions(options | TRANSACTION_OPTIONS_TX_HASH_SIGN);
transaction.setOptions(TransactionOptions.withOptions({ guarded: true }));
transaction.setOptions(TransactionOptions.withOptions({
    guarded: true,
    ...(isLedger ? { hashSign: true } : {})
}));

These functions, setVersion and setOptions, remain available (legacy).

andreibancioiu · 2024-03-11T14:58:16Z

src/transaction.ts

+    public version: number;
+
+    /**
+     * The options field, useful for describing different settings available for transactions.
+     */
+    public options: number;


Breaking change.

andreibancioiu · 2024-03-11T15:03:59Z

src/transaction.ts

-        return feeForMove;
-    }
+    computeBytesForSigning(transaction: ITransaction): Uint8Array {
+        // TODO: do some checks for the transaction e.g. sender, chain ID etc.


We should not forget about this.

popenta · 2024-03-12T11:33:42Z

src/transaction.ts

+    /**
+     * The address of the sender.
+     */
+    public sender: string;
+
+    /**
+     * The address of the receiver.
+     */
+    public receiver: string;


Now that i think about, maybe in the js doc should be mentioned that the bech32 representation of an address should be provided. (optional)

popenta · 2024-03-12T11:34:53Z

src/transaction.ts

+    /**
+     * The address of the guardian.
+     */
+    public guardian: string;


same comment as above.

popenta · 2024-03-12T11:36:34Z

src/transaction.ts

+    public constructor({
+        nonce,
+        value,
+        sender,
+        receiver,
+        senderUsername,
+        receiverUsername,
+        gasPrice,
+        gasLimit,
+        data,
+        chainID,
+        version,
+        options,
+        guardian,
+        signature,
+        guardianSignature,
+    }: {
+        nonce?: INonce | bigint;
+        value?: ITransactionValue | bigint;
+        sender?: IAddress | string;
+        receiver?: IAddress | string;
+        senderUsername?: string;
+        receiverUsername?: string;
+        gasPrice?: IGasPrice | bigint;
+        gasLimit?: IGasLimit | bigint;
+        data?: ITransactionPayload | Uint8Array;
+        chainID?: IChainID | string;
+        version?: ITransactionVersion | number;
+        options?: ITransactionOptions | number;
+        guardian?: IAddress | string;
+        signature?: Uint8Array;
+        guardianSignature?: Uint8Array;
+    }) {


perhaps change the constructor as bellow? to be aligned with the transaction factories.

constructor(options: {...})

popenta · 2024-03-12T12:09:42Z

src/transaction.ts

+        sender?: IAddress | string;
+        receiver?: IAddress | string;


shouldn't we have mandatory fields on the constructor? Like sender, receiver, gas limit and chain?

Indeed, fixed. Reverted, and now we have the same required fields as in PY (as in JS v12, as well): sender, receiver, chain ID and gas limit.

popenta · 2024-03-12T12:38:36Z

src/smartcontracts/smartContract.ts

@@ -127,7 +127,7 @@ export class SmartContract implements ISmartContract {
        const bytecode = Buffer.from(code.toString(), 'hex');
        const metadataAsJson = this.getMetadataPropertiesAsObject(codeMetadata);

-        const nextTx = scNextTransactionFactory.createTransactionForDeploy({
+        const transaction = scNextTransactionFactory.createTransactionForDeploy({


also rename scNextTransactionFactory

src/converters/transactionsConverter.ts

popenta · 2024-03-14T13:21:49Z

src/converters/transactionsConverter.ts

+
+    public plainObjectToTransaction(object: IPlainTransactionObject): Transaction {
+        const transaction = new Transaction({
+            nonce: Number(object.nonce),


this can be BigInt

Indeed. Fixed.

popenta · 2024-03-14T13:24:08Z

src/transaction.ts

-  }
-}
+    private addressAsBech32(address: IAddress | string): string {
+        return typeof address === "string" ? address : address.bech32();


I hope people pass the correct string representation of the address. If people pass the address as a hex string this will not work as expected.

Indeed. In the specs, we have transaction.sender, transaction.receiver as strings.

andreibancioiu added 4 commits March 10, 2024 22:10

Merge TransactionNext into existing Transaction.

16c81fd

Use existing Transaction instead of NextTransaction.

4ee60b4

No more "TransactionNext".

7f55dd5

No more transactionNext.

21ff0ee

andreibancioiu marked this pull request as ready for review March 11, 2024 10:05

andreibancioiu added 2 commits March 11, 2024 15:45

Merge tests.

413c388

Adjust / rename tests.

4506ee0

andreibancioiu self-assigned this Mar 11, 2024

andreibancioiu commented Mar 11, 2024

View reviewed changes

popenta reviewed Mar 12, 2024

View reviewed changes

andreibancioiu added 7 commits March 13, 2024 14:39

Merge branch 'feat/next' into same-tx

228a881

Merge branch 'feat/next' into same-tx

05bdafe

Fix after review (partial).

c47cb8e

Fix after review (partial).

82f088c

Fix after review.

6ddacc4

SerializeForSigning() now calls TransactionComputer.

023ffa4

Refactor getHash(), mark it as legacy.

c16e60e

andreibancioiu changed the title ~~Merge "TransactionNext" into the existing "Transaction" class~~ (small breaking change) Merge "TransactionNext" into the existing "Transaction" class Mar 13, 2024

andreibancioiu changed the title ~~(small breaking change) Merge "TransactionNext" into the existing "Transaction" class~~ (small breaking change) Merge v13's "TransactionNext" into the existing "Transaction" class Mar 13, 2024

andreibancioiu added 4 commits March 13, 2024 18:07

Simplify toPlainObject() for signing.

20def8a

Minor refactoring.

d19f4b0

Move conversion logic to converters package.

6a6a810

Add test.

4985058

danielailie reviewed Mar 14, 2024

View reviewed changes

src/converters/transactionsConverter.ts Show resolved Hide resolved

danielailie previously approved these changes Mar 14, 2024

View reviewed changes

Fix after review.

4dc94e3

andreibancioiu dismissed danielailie’s stale review via 4dc94e3 March 14, 2024 12:56

danielailie previously approved these changes Mar 14, 2024

View reviewed changes

radumojic previously approved these changes Mar 14, 2024

View reviewed changes

popenta reviewed Mar 14, 2024

View reviewed changes

Fix after review.

afad158

andreibancioiu dismissed stale reviews from radumojic and danielailie via afad158 March 14, 2024 13:31

danielailie approved these changes Mar 14, 2024

View reviewed changes

popenta approved these changes Mar 14, 2024

View reviewed changes

radumojic approved these changes Mar 14, 2024

View reviewed changes

andreibancioiu merged commit ea1499a into feat/next Mar 14, 2024
1 check passed

andreibancioiu deleted the same-tx branch March 14, 2024 13:49

This was referenced Mar 24, 2024

v13: Merge feat/next into main #347

Merged

Migration support: "v12" to "v13" (April 2024) #392

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(small breaking change) Merge v13's "TransactionNext" into the existing "Transaction" class #396

(small breaking change) Merge v13's "TransactionNext" into the existing "Transaction" class #396

andreibancioiu commented Mar 11, 2024 •

edited

Loading

andreibancioiu Mar 11, 2024

andreibancioiu Mar 11, 2024

popenta Mar 12, 2024

andreibancioiu Mar 13, 2024

popenta Mar 12, 2024

andreibancioiu Mar 13, 2024

popenta Mar 12, 2024

andreibancioiu Mar 13, 2024

popenta Mar 12, 2024

andreibancioiu Mar 13, 2024

popenta Mar 12, 2024

andreibancioiu Mar 13, 2024

popenta Mar 14, 2024

andreibancioiu Mar 14, 2024

popenta Mar 14, 2024

andreibancioiu Mar 14, 2024

(small breaking change) Merge v13's "TransactionNext" into the existing "Transaction" class #396

(small breaking change) Merge v13's "TransactionNext" into the existing "Transaction" class #396

Conversation

andreibancioiu commented Mar 11, 2024 • edited Loading

Breaking (but small) change on tx.version and tx.options

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

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

andreibancioiu commented Mar 11, 2024 •

edited

Loading

Breaking (but small) change on `tx.version` and `tx.options`