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.

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

[Tests] Transaction submission test suite. The initial attempt at testing all authenticators, payloads and argument types #34

Merged
merged 8 commits into from
Oct 14, 2023

Conversation

xbtmatt
Copy link
Contributor

@xbtmatt xbtmatt commented Oct 12, 2023

Test suite

TODO:

  • Add 1 - n signers for the functions
  • Verify the order of signers in the contract with assertions

Argument types

  • basic argument types
  • vector argument types
  • option argument types

Authenticators & payloads

Ed25519 (1 sender, 1 signer)

  • public entry function
  • private entry function
  • script

Secp256k1 (1 sender, 1 signer)

  • public entry function
  • private entry function
  • script

MultiEd25519 (k-of-n signers)

  • TODO

Multi Agent (1 sender, n - 1 secondary signers, n signers)

  • public entry function Ed25519
  • private entry function Ed25519
  • script Ed25519
  • public entry function Secp256k1
  • private entry function Secp256k1
  • script Secp256k1

Fee Payer (1 sender, 1 fee payer, 0 - n signers)

  • public entry function Ed25519
  • private entry function Ed25519
  • script Ed25519
  • public entry function Secp256k1
  • private entry function Secp256k1
  • script Secp256k1

Miscellaneous:

  • view function of all the argument types listed above
  • type tags, including various nested generics

@@ -439,7 +439,7 @@ export class MoveObject extends Serializable implements TransactionArgument {
if (value instanceof AccountAddress) {
this.value = value;
} else {
this.value = AccountAddress.fromHexInput({ input: value });
this.value = AccountAddress.fromHexInputRelaxed({ input: value });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this changing?

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 would like to use relaxed if we can, I was running into a bunch of errors because of it- basically what we were talking about with daniel

I was just putting it here as a fix so I didn't have to fix every instance of an object in the test suite (I wrote it down as feedback/things to think about in the doc), but yea, the issue was I was getting errors from addresses I was getting from the API on chain

@xbtmatt xbtmatt force-pushed the transaction-arguments branch from 5b9f02f to 5875397 Compare October 13, 2023 07:08
@xbtmatt
Copy link
Contributor Author

xbtmatt commented Oct 13, 2023

TODO: I think we could add something like these (just with single signer, ed25519) as a bcs-examples.ts file to show people how to use the new stuff, since it is very comprehensive

@xbtmatt xbtmatt changed the title [Tests] Transaction submission test suite. All combinations of authenticators, payloads and argument types [Tests] Transaction submission test suite. The initial attempt at tseting all authenticators, payloads and argument types Oct 13, 2023
@xbtmatt xbtmatt changed the title [Tests] Transaction submission test suite. The initial attempt at tseting all authenticators, payloads and argument types [Tests] Transaction submission test suite. The initial attempt at testing all authenticators, payloads and argument types Oct 13, 2023
@xbtmatt xbtmatt marked this pull request as ready for review October 13, 2023 22:40
@xbtmatt xbtmatt requested review from 0xmaayan, 0xmigo, heliuchuan and a team as code owners October 13, 2023 22:40
@xbtmatt xbtmatt force-pushed the transaction-arguments branch from 26f9415 to fb1f9c2 Compare October 14, 2023 00:11
Copy link
Collaborator

@gregnazario gregnazario left a comment

Choose a reason for hiding this comment

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

This is a lot of code. Let's please break this up into functions in the future for each step rather than have a really really big beforeAll

UserTransactionResponse,
} from "../../../src/types";

jest.setTimeout(30000);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a test that's actually taking 30 seconds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I think I just changed it to 30 to make sure it wouldn't fail, I think I can do like 10 or 15

Comment on lines +53 to +57
account1 = Account.generate();
account2 = Account.generate();
account3 = Account.generate();
account4 = Account.generate();
account5 = Account.generate();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably would have been easier just to have an array of accounts.

for (const account of accounts) {
await aptos.fundAccount({
accountAddress: account.accountAddress.data,
amount: 100_000_000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a constant for this now

Comment on lines +148 to +170
if (secondarySignerAddresses.length == 0) {
const response = await aptos.generateTransaction({
sender: sender.accountAddress.toString(),
data: {
function: `0x${account1.accountAddress.toStringWithoutPrefix()}::tx_args_module::${functionName}`,
type_arguments: typeArgs,
arguments: args,
},
feePayerAddress: feePayerAddress!.data,
});
return response;
} else if (feePayerAddress === undefined) {
return await aptos.generateTransaction({
sender: sender.accountAddress.toString(),
data: {
function: `0x${account1.accountAddress.toStringWithoutPrefix()}::tx_args_module::${functionName}`,
type_arguments: typeArgs,
arguments: args,
},
secondarySignerAddresses: secondarySignerAddresses.map((address) => address.data),
});
} else {
return await aptos.generateTransaction({
Copy link
Collaborator

Choose a reason for hiding this comment

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

:/ super fragile, but we'll have to come back

Copy link
Contributor Author

@xbtmatt xbtmatt Oct 14, 2023

Choose a reason for hiding this comment

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

Yeah, the only reason I did it like this is because the types weren't resolving correctly, otherwise it should have correctly resolved to a general type with no if/else blocks

return await aptos.generateTransaction({
sender: sender.accountAddress.toString(),
data: {
function: `0x${account1.accountAddress.toStringWithoutPrefix()}::tx_args_module::${functionName}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just do toString() with the prefix instead of adding the 0x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was wondering why I would ever do this and remembered the issue is the type is that 0x${...}::${...} type, so it gets mad otherwise


const accounts = [sender, account2, account3, account4, account5];
// sign wtih an account its address is in secondarySignerAddresses OR if it's the sender
// TODO: Fix the ugly
Copy link
Collaborator

Choose a reason for hiding this comment

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

:/

tests/e2e/api/transaction_arguments.test.ts Show resolved Hide resolved
Copy link
Collaborator

@0xmaayan 0xmaayan left a comment

Choose a reason for hiding this comment

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

it is a big test. maybe we should put all transactions tests under a transactions folder. can also have a helper file for transaction specific tests

@@ -439,7 +439,7 @@ export class MoveObject extends Serializable implements TransactionArgument {
if (value instanceof AccountAddress) {
this.value = value;
} else {
this.value = AccountAddress.fromHexInput({ input: value });
this.value = AccountAddress.fromHexInputRelaxed({ input: value });
Copy link
Collaborator

Choose a reason for hiding this comment

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

interesting... curios why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I was getting objects from the API, it returned it with no leading 0, meaning I needed to sanitize my input. I then realized I don't see in any case why someone wouldn't want the "relaxed" version when using it as input.

You generally would only use a MoveObject to serialize an entry function argument, so the end serialization result is the same regardless of whether or not you input a 0x0... or 0x..., so fromHexInputRelaxed makes more sense to me than having people run into errors 1/16 times because they didn't expect the address they passed in to to fail the parsing test

transaction: rawTxn,
senderAuthenticator: signedTxn,
});
const _ = await aptos.waitForTransaction({
Copy link
Collaborator

Choose a reason for hiding this comment

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

does it complain if you dont do const _ = ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no I'm not sure why I did that tbh, removed it

@gregnazario gregnazario merged commit 46bb53b into main Oct 14, 2023
5 checks passed
@gregnazario gregnazario deleted the transaction-arguments branch October 14, 2023 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants