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

ethereum: added e2e tests #2218

Merged
merged 14 commits into from
Nov 22, 2024
Merged

ethereum: added e2e tests #2218

merged 14 commits into from
Nov 22, 2024

Conversation

aramikm
Copy link
Collaborator

@aramikm aramikm commented Nov 13, 2024

Goal

The goal of this PR is

Closes #2203

Discussion

  • added e2e tests for more usecase for ethereum keys
  • Some clean up and refactoring
  • Added eslint rules to enforce correct usage

Checklist

  • e2e Tests added?

Base automatically changed from ethereum_style_account_support to main November 15, 2024 17:10
@aramikm aramikm marked this pull request as ready for review November 15, 2024 21:35
@aramikm aramikm requested a review from wilwade as a code owner November 15, 2024 21:35
@@ -1,3 +1,4 @@
/* eslint-disable no-restricted-syntax */
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Only ignoring the custom rule in here

@aramikm aramikm changed the title ethereum: added e2e tests [DO NOT Merge] ethereum: added e2e tests Nov 15, 2024
@@ -51,6 +51,19 @@ export default tseslint.config(
},
],
'allow-namespace': 'off',
'no-restricted-syntax': [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

adding new rules so going forward we have to use correct functions

Copy link
Collaborator

@saraswatpuneet saraswatpuneet left a comment

Choose a reason for hiding this comment

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

read through, lgtm 💯

Comment on lines +65 to +93
const schema = {
type: 'record',
name: 'Post',
fields: [
{ name: 'title', type: { name: 'Title', type: 'string' } },
{ name: 'content', type: { name: 'Content', type: 'string' } },
{ name: 'fromId', type: { name: 'DSNPId', type: 'fixed', size: 8 } },
{ name: 'objectId', type: 'DSNPId' },
],
};

schemaId = await ExtrinsicHelper.getOrCreateSchemaV3(
keys,
schema,
'AvroBinary',
'OnChain',
[],
'test.grantDelegation'
);

schemaId2 = await ExtrinsicHelper.getOrCreateSchemaV3(
keys,
schema,
'AvroBinary',
'OnChain',
[],
'test.grantDelegationSecond'
);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: probably don't need to create schemas any more, unless actually testing the schemas pallet, as mainnet schemas should be available on all chains now

Copy link
Collaborator Author

@aramikm aramikm Nov 21, 2024

Choose a reason for hiding this comment

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

yeah this is getting the schemaId if it already exists but I agree that we can refactor all these schema creation everywhere and simplify it but looks like a wider effort outside of the scope of this ticket. Will create an issue for it

Copy link
Collaborator

@JoeCap08055 JoeCap08055 left a comment

Choose a reason for hiding this comment

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

One general comment: it seems you've cloned some of the tests solely to validate that they work as well with Ethereum-style keys. Might it be simpler to just convert the existing tests to table tests that run the same test using different keys?

@aramikm
Copy link
Collaborator Author

aramikm commented Nov 20, 2024

@JoeCap08055 There are some minor other differences between ethereum ones and other ones besides the keys used. One of the benefits of having them in separate files is that I can just change the test selector to *.ethereum.test.ts and it would only run ethereum related tests but open to other suggestions

@JoeCap08055
Copy link
Collaborator

@JoeCap08055 There are some minor other differences between ethereum ones and other ones besides the keys used. One of the benefits of having them in separate files is that I can just change the test selector to *.ethereum.test.ts and it would only run ethereum related tests but open to other suggestions

If there's a good reason, then I'm fine with it.

Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

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

Couple of non-blocking comments from me, if you can add those extra checks that @JoeCap08055 suggested I think that would be a good idea.

e2e/miscellaneous/balance.ethereum.test.ts Outdated Show resolved Hide resolved
e2e/scaffolding/helpers.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@mattheworris mattheworris left a comment

Choose a reason for hiding this comment

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

  • Read through changes

🚢 it!

@aramikm aramikm merged commit f70489c into main Nov 22, 2024
28 checks passed
@aramikm aramikm deleted the ethereum_e2e branch November 22, 2024 22:20
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.

Verify Support for Address20
5 participants