Added RelayedV3 method in RelayedTransactionsFactory #460

popenta · 2024-06-10T08:30:24Z

No description provided.

andreibancioiu · 2024-06-10T13:16:26Z

src/converters/transactionsConverter.ts

@@ -28,6 +28,10 @@ export class TransactionsConverter {
            guardian: transaction.guardian ? transaction.guardian : undefined,
            signature: this.toHexOrUndefined(transaction.signature),
            guardianSignature: this.toHexOrUndefined(transaction.guardianSignature),
+            relayer: transaction.relayer ? transaction.relayer : undefined,
+            innerTransactions: transaction.innerTransactions.length


Is it an option to have innerTransactions as an empty array if there's no item?

Here, below, and in other places.

Later comment: actually, it's good that toPlainObject() returns undefined if these new fields are missing 👍

Otherwise, downstream, some errors might have occurred. E.g. sending toPlainObject() to a gateway etc. (while these new fields - even if empty - aren't supported yet).

I am not sure, here maybe it could work but in other places we check the length and make it undefined anyway if there's no inner transaction(e.g. serializing for signing). Is there a reason why we'd make it an empty array instead of undefined?

It's good as it is (see the later comment) 👍

Generally speaking, reason for empty array instead of undefined: leads to less errors in client code (e.g. better to iterate over empty arrays, than pre-checking if an array is defined, then iterate over it).

But in this context, it's good.

andreibancioiu · 2024-06-10T13:20:18Z

src/transaction.ts

@@ -139,6 +152,9 @@ export class Transaction {

        this.signature = options.signature || Buffer.from([]);
        this.guardianSignature = options.guardianSignature || Buffer.from([]);
+
+        this.relayer = options.relayer || "";
+        this.innerTransactions = options.innerTransactions || [];


andreibancioiu · 2024-06-11T08:10:47Z

src/transactionsFactories/relayedTransactionsFactory.ts

+            chainID: this.config.chainID,
+            gasLimit: gasLimit,
+            innerTransactions: options.innerTransactions,
+        });


Oh, so the relayer field does not have to be set, it seems?

https://github.com/multiversx/mx-sdk-py/blob/feat/next/multiversx_sdk/core/transactions_factories/relayed_transactions_factory.py#L101

the relayer field is only set for the inner transactions

andreibancioiu · 2024-06-11T08:12:56Z

src/interface.ts

+    relayer?: string;
+    innerTransactions?: IPlainTransactionObject[];


Indeed, optional, to have this as a non-breaking change.

andreibancioiu · 2024-06-11T08:13:52Z

src/interface.ts

@@ -104,4 +106,6 @@ export interface ITransaction {
    guardian: string;
    signature: Uint8Array;
    guardianSignature: Uint8Array;
+    relayer: string;


Breaking change? Can we handle this differently?

Well, theoretically it is a breaking change, but i assume everybody uses our Transaction class so it shouldn't be a big deal. Did we make a major release when guardians were added as well?

For guardians, indeed, we did (though it wasn't the sole reason): https://github.com/multiversx/mx-sdk-js-core/releases/tag/v12.0.1.

For practicality, yes, makes sense to treat it as a not-really-breaking-change, agree 👍

Scenario for breaking change (hypothetically speaking): some client code references and uses the interface - even we are thinking of it as an internal interface.

Now I see, in sdk-core it's only used in TransactionWatcher. Then, maybe, even have the Transaction class use the concrete Transaction DTO as inner transactions?

andreibancioiu · 2024-06-11T08:18:51Z

src/transactionComputer.ts

        };
    }

+    private toPlainObject(transaction: ITransaction): any {


This approach is different from the one in PY.

https://github.com/multiversx/mx-sdk-py/blob/feat/next/multiversx_sdk/core/transaction_computer.py#L94

Can also stay as it is, can also be refactored for symmetry.

Refactored it for symmetry. Also renamed the method.

andreibancioiu · 2024-06-11T08:21:28Z

src/proto/serializer.spec.ts

+            serializedTransaction.toString("hex"),
+            "08cc011209000de0b6b3a76400001a200139472eff6886771a982f3083da5d421f24c29181e63888228dc81ca60d69e12205616c6963652a20b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba32056361726f6c388094ebdc0340d0860352015458026240901a6a974d6ab36546e7881c6e0364ec4c61a891aa70e5eb60f818d6c92a39cfa0beac6fab73f503853cfe8fe6149b4be207ddb93788f8450d75a07fa8759d06820120b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba8a01b10108cc011209000de0b6b3a76400001a200139472eff6886771a982f3083da5d421f24c29181e63888228dc81ca60d69e12205616c6963652a20b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba32056361726f6c388094ebdc0340d086035201545802624051e6cd78fb3ab4b53ff7ad6864df27cb4a56d70603332869d47a5cf6ea977c30e696103e41e8dddf2582996ad335229fdf4acb726564dbc1a0bc9e705b511f06",
+        );
+    });


Now I see that we have nonce = 204 for both the inner and the relayed transaction. Also same sender & receiver in inner vs. relayed. In PY as well.

https://github.com/multiversx/mx-sdk-py/blob/feat/next/multiversx_sdk/core/proto/transaction_serializer_test.py

Might lead to some ambiguity for some readers.

Should we change this? In JS & PY?

I did it like this to match the test in sdk-py. We could leave it like this for the moment and replace it in the near future with some more explicit tests.

andreibancioiu · 2024-06-11T08:24:06Z

src/proto/serializer.ts

 import { bigIntToBuffer } from "../smartcontracts/codec/utils";
 import { Transaction } from "../transaction";

+const proto = require("./compiled").proto;


This was problematic in the past.

#295

Can we undo for the moment, then think of a better solution in a separate PR?

Sure, done!

andreibancioiu · 2024-06-11T08:25:09Z

src/proto/serializer.ts


        let protoTransaction = new proto.Transaction({
            // mx-chain-go's serializer handles nonce == 0 differently, thus we treat 0 as "undefined".
-            Nonce: transaction.getNonce().valueOf() ? transaction.getNonce().valueOf() : undefined,
-            Value: this.serializeTransactionValue(transaction.getValue()),
+            Nonce: Number(transaction.nonce) ? Number(transaction.nonce) : undefined,


andreibancioiu · 2024-06-11T08:27:48Z

src/transactionsFactories/relayedTransactionsFactory.spec.ts

+            sender: bob.address.toBech32(),
+            receiver: bob.address.toBech32(),


Could have been alice to bob.

did it to match the test in sdk-py

popenta added 2 commits June 10, 2024 11:24

implement relayed v3 in relayed transactions factory

eb8d4fd

align proto schema

b45f3a9

popenta self-assigned this Jun 10, 2024

align relayer field in proto schema

4a013c2

andreibancioiu self-requested a review June 10, 2024 08:38

CiprianDraghici previously approved these changes Jun 10, 2024

View reviewed changes

andreibancioiu reviewed Jun 11, 2024

View reviewed changes

fixes after review

9b31e53

popenta dismissed CiprianDraghici’s stale review via 9b31e53 June 11, 2024 11:25

andreibancioiu approved these changes Jun 11, 2024

View reviewed changes

CiprianDraghici approved these changes Jun 11, 2024

View reviewed changes

popenta merged commit 152b90a into feat/next Jun 11, 2024
1 check passed

popenta deleted the relayed-v3 branch June 11, 2024 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added RelayedV3 method in RelayedTransactionsFactory #460

Added RelayedV3 method in RelayedTransactionsFactory #460

popenta commented Jun 10, 2024

andreibancioiu Jun 10, 2024

andreibancioiu Jun 10, 2024

andreibancioiu Jun 11, 2024 •

edited

Loading

popenta Jun 11, 2024

andreibancioiu Jun 11, 2024

andreibancioiu Jun 10, 2024

andreibancioiu Jun 11, 2024

popenta Jun 11, 2024

andreibancioiu Jun 11, 2024

andreibancioiu Jun 11, 2024

popenta Jun 11, 2024

andreibancioiu Jun 11, 2024 •

edited

Loading

andreibancioiu Jun 11, 2024

popenta Jun 11, 2024

andreibancioiu Jun 11, 2024

andreibancioiu Jun 11, 2024

popenta Jun 11, 2024

andreibancioiu Jun 11, 2024

popenta Jun 11, 2024

andreibancioiu Jun 11, 2024

andreibancioiu Jun 11, 2024

popenta Jun 11, 2024

		relayer?: string;
		innerTransactions?: IPlainTransactionObject[];

		sender: bob.address.toBech32(),
		receiver: bob.address.toBech32(),

Added RelayedV3 method in RelayedTransactionsFactory #460

Added RelayedV3 method in RelayedTransactionsFactory #460

Conversation

popenta commented Jun 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu Jun 11, 2024 • edited Loading

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 Jun 11, 2024 • edited Loading

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 Jun 11, 2024 •

edited

Loading

andreibancioiu Jun 11, 2024 •

edited

Loading