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.

Sign up for GitHub

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

Jump to bottom

Added RelayedV3 method in RelayedTransactionsFactory #460

Merged
merged 4 commits into from
Jun 11, 2024
Merged

Added RelayedV3 method in RelayedTransactionsFactory #460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eb8d4fd
implement relayed v3 in relayed transactions factory
popenta Jun 10, 2024
b45f3a9
align proto schema
popenta Jun 10, 2024
4a013c2
align relayer field in proto schema
popenta Jun 10, 2024
9b31e53
fixes after review
popenta Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/converters/transactionsConverter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Here, below, and in other places.

Copy link
Contributor

@andreibancioiu andreibancioiu Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

? transaction.innerTransactions.map((tx) => this.transactionToPlainObject(tx))
: undefined,
};

return plainObject;
Expand Down Expand Up @@ -58,6 +62,10 @@ export class TransactionsConverter {
options: Number(object.options),
signature: this.bufferFromHex(object.signature),
guardianSignature: this.bufferFromHex(object.guardianSignature),
relayer: object.relayer,
innerTransactions: object.innerTransactions
? object.innerTransactions.map((tx) => this.plainObjectToTransaction(tx))
: undefined,
});

return transaction;
Expand Down
79 changes: 79 additions & 0 deletions src/converters/transactionsConverters.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,85 @@ describe("test transactions converter", async () => {
guardian: undefined,
signature: undefined,
guardianSignature: undefined,
relayer: undefined,
innerTransactions: undefined,
});
});

it("converts relayedV3 transaction to plain object and back", () => {
const converter = new TransactionsConverter();

const innerTx = new Transaction({
nonce: 90,
value: BigInt("123456789000000000000000000000"),
sender: "erd1qyu5wthldzr8wx5c9ucg8kjagg0jfs53s8nr3zpz3hypefsdd8ssycr6th",
receiver: "erd1spyavw0956vq68xj8y4tenjpq2wd5a9p2c6j8gsz7ztyrnpxrruqzu66jx",
senderUsername: "alice",
receiverUsername: "bob",
gasPrice: 1000000000,
gasLimit: 80000,
data: Buffer.from("hello"),
chainID: "localnet",
version: 2,
relayer: "erd1k2s324ww2g0yj38qn2ch2jwctdy8mnfxep94q9arncc6xecg3xaq6mjse8",
});

const relayedTx = new Transaction({
nonce: 77,
value: BigInt("0"),
sender: "erd1k2s324ww2g0yj38qn2ch2jwctdy8mnfxep94q9arncc6xecg3xaq6mjse8",
receiver: "erd1k2s324ww2g0yj38qn2ch2jwctdy8mnfxep94q9arncc6xecg3xaq6mjse8",
gasPrice: 1000000000,
gasLimit: 50000,
chainID: "localnet",
version: 2,
innerTransactions: [innerTx],
});

const plainObject = converter.transactionToPlainObject(relayedTx);
const restoredTransaction = converter.plainObjectToTransaction(plainObject);

assert.deepEqual(plainObject, relayedTx.toPlainObject());
assert.deepEqual(restoredTransaction, Transaction.fromPlainObject(plainObject));
assert.deepEqual(restoredTransaction, relayedTx);
assert.deepEqual(plainObject, {
nonce: 77,
value: "0",
sender: "erd1k2s324ww2g0yj38qn2ch2jwctdy8mnfxep94q9arncc6xecg3xaq6mjse8",
receiver: "erd1k2s324ww2g0yj38qn2ch2jwctdy8mnfxep94q9arncc6xecg3xaq6mjse8",
senderUsername: undefined,
receiverUsername: undefined,
gasPrice: 1000000000,
gasLimit: 50000,
data: undefined,
chainID: "localnet",
version: 2,
options: undefined,
guardian: undefined,
signature: undefined,
guardianSignature: undefined,
relayer: undefined,
innerTransactions: [
{
nonce: 90,
value: "123456789000000000000000000000",
sender: "erd1qyu5wthldzr8wx5c9ucg8kjagg0jfs53s8nr3zpz3hypefsdd8ssycr6th",
receiver: "erd1spyavw0956vq68xj8y4tenjpq2wd5a9p2c6j8gsz7ztyrnpxrruqzu66jx",
senderUsername: "YWxpY2U=",
receiverUsername: "Ym9i",
gasPrice: 1000000000,
gasLimit: 80000,
data: "aGVsbG8=",
chainID: "localnet",
version: 2,
options: undefined,
guardian: undefined,
signature: undefined,
guardianSignature: undefined,
relayer: "erd1k2s324ww2g0yj38qn2ch2jwctdy8mnfxep94q9arncc6xecg3xaq6mjse8",
innerTransactions: undefined,
},
],
});
});

Expand Down
4 changes: 4 additions & 0 deletions src/interface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ export interface IPlainTransactionObject {
options?: number;
signature?: string;
guardianSignature?: string;
relayer?: string;
innerTransactions?: IPlainTransactionObject[];
Comment on lines +27 to +28
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

export interface ISignature {
Expand Down Expand Up @@ -104,4 +106,6 @@ export interface ITransaction {
guardian: string;
signature: Uint8Array;
guardianSignature: Uint8Array;
relayer: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Breaking change? Can we handle this differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@andreibancioiu andreibancioiu Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

innerTransactions: ITransaction[];
}
77 changes: 77 additions & 0 deletions src/proto/compiled.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

39 changes: 39 additions & 0 deletions src/proto/serializer.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { TokenTransfer } from "../tokens";
import { Transaction } from "../transaction";
import { TransactionPayload } from "../transactionPayload";
import { ProtoSerializer } from "./serializer";
import { TransactionComputer } from "../transactionComputer";

describe("serialize transactions", () => {
let wallets: Record<string, TestWallet>;
Expand Down Expand Up @@ -145,4 +146,42 @@ describe("serialize transactions", () => {
"08cc011209000de0b6b3a76400001a200139472eff6886771a982f3083da5d421f24c29181e63888228dc81ca60d69e12205616c6963652a20b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba32056361726f6c388094ebdc0340d086035201545802624051e6cd78fb3ab4b53ff7ad6864df27cb4a56d70603332869d47a5cf6ea977c30e696103e41e8dddf2582996ad335229fdf4acb726564dbc1a0bc9e705b511f06",
);
});

it("serialize with inner transactions", async () => {
const innerTransaction = new Transaction({
nonce: 204,
value: "1000000000000000000",
sender: Address.fromBech32("erd1k2s324ww2g0yj38qn2ch2jwctdy8mnfxep94q9arncc6xecg3xaq6mjse8"),
receiver: Address.fromBech32("erd1qyu5wthldzr8wx5c9ucg8kjagg0jfs53s8nr3zpz3hypefsdd8ssycr6th"),
senderUsername: "carol",
receiverUsername: "alice",
gasLimit: 50000,
chainID: "T",
});

const signer = wallets.carol.signer;
const txComputer = new TransactionComputer();
innerTransaction.signature = await signer.sign(txComputer.computeBytesForSigning(innerTransaction));

const relayedTransaction = new Transaction({
nonce: 204,
value: "1000000000000000000",
sender: Address.fromBech32("erd1k2s324ww2g0yj38qn2ch2jwctdy8mnfxep94q9arncc6xecg3xaq6mjse8"),
receiver: Address.fromBech32("erd1qyu5wthldzr8wx5c9ucg8kjagg0jfs53s8nr3zpz3hypefsdd8ssycr6th"),
senderUsername: "carol",
receiverUsername: "alice",
gasLimit: 50000,
chainID: "T",
relayer: wallets["carol"].address.toBech32(),
innerTransactions: [innerTransaction],
});

relayedTransaction.signature = await signer.sign(txComputer.computeBytesForSigning(relayedTransaction));

const serializedTransaction = serializer.serializeTransaction(relayedTransaction);
assert.equal(
serializedTransaction.toString("hex"),
"08cc011209000de0b6b3a76400001a200139472eff6886771a982f3083da5d421f24c29181e63888228dc81ca60d69e12205616c6963652a20b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba32056361726f6c388094ebdc0340d0860352015458026240901a6a974d6ab36546e7881c6e0364ec4c61a891aa70e5eb60f818d6c92a39cfa0beac6fab73f503853cfe8fe6149b4be207ddb93788f8450d75a07fa8759d06820120b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba8a01b10108cc011209000de0b6b3a76400001a200139472eff6886771a982f3083da5d421f24c29181e63888228dc81ca60d69e12205616c6963652a20b2a11555ce521e4944e09ab17549d85b487dcd26c84b5017a39e31a3670889ba32056361726f6c388094ebdc0340d086035201545802624051e6cd78fb3ab4b53ff7ad6864df27cb4a56d70603332869d47a5cf6ea977c30e696103e41e8dddf2582996ad335229fdf4acb726564dbc1a0bc9e705b511f06",
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change this? In JS & PY?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

});
Loading
Loading