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

feat: test the failed parachain extrinsic handling in worker #2543

Merged

Conversation

felixfaisal
Copy link
Member

@felixfaisal felixfaisal commented Mar 4, 2024

So the idea of this PR was:

  1. Call link-identity for Bob
  2. Wait for it to be succesful and capture the id-graph hash
  3. Call link-identity but use a delegate signer, the extrinsic will fail since the delegate is not authorized
  4. We wait, and capture the id-graph hash, no change in id-graph-hash would signify that the failed extrinsic did not execute.

Copy link

linear bot commented Mar 4, 2024

@felixfaisal felixfaisal changed the base branch from dev to p-553-link-identity-in-cli March 4, 2024 06:21
@felixfaisal felixfaisal changed the base branch from p-553-link-identity-in-cli to dev March 4, 2024 07:02
@felixfaisal felixfaisal changed the base branch from dev to p-553-link-identity-in-cli March 4, 2024 07:04
@felixfaisal felixfaisal changed the base branch from p-553-link-identity-in-cli to dev March 5, 2024 06:12
@felixfaisal felixfaisal requested review from BillyWooo, Kailai-Wang, a team and kziemianek and removed request for BillyWooo March 11, 2024 05:00
Copy link
Contributor

@jonalvarezz jonalvarezz left a comment

Choose a reason for hiding this comment

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

Naïve question: why not a ts-test? i.e. when should we prefer cli test over ts-test?

@felixfaisal
Copy link
Member Author

Naïve question: why not a ts-test? i.e. when should we prefer cli test over ts-test?

A good rule of thumb would be if it is assigned to rust dev, then cli would be preferred xD
However, we did have a few CI tests that make use of CLI, and have been removed, so it's not exactly reinventing the wheel since the support always has been there. This is basically my rationale.

@Kailai-Wang
Copy link
Collaborator

Naïve question: why not a ts-test? i.e. when should we prefer cli test over ts-test?

This is actually a good one.
I'd say either will do its job, but rust CLI generally provides more flexibility and is simpler (from code) as it's closer to call existing rust functions. So achieving the same functionality requires more code in ts :D

Upstream doesn't have ts-test btw, we need that because we want some e2e test and our F/E is ofc in ts

Copy link
Collaborator

@Kailai-Wang Kailai-Wang left a comment

Choose a reason for hiding this comment

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

Thanks!

While I understand what the code tries to do, I'm not sure if it meets the test goal. We want to make sure that failed extrinsics don't even get executed - imagine you transfer 1m tokens to someone and it fails as you have insufficient balance, the indirect call executor should detect that the extrinsic fails and never try to parse and execute it.

This PR tests however, when an error is thrown out during the execution of the II dispatcher, it won't write any wrong state - which is something good to test, but not the same as what we wanted to test IMO

tee-worker/cli/lit_test_failed_parentchain_extrinsic.sh Outdated Show resolved Hide resolved
@felixfaisal
Copy link
Member Author

This PR tests however, when an error is thrown out during the execution of the II dispatcher, it won't write any wrong state - which is something good to test, but not the same as what we wanted to test IMO

I don't think so it is testing the error thrown out during the execution of the II dispatch. It is doing the former.

The failed extrinsic gets parsed but is not executed, Let me know if I'm missing something here.

@kziemianek
Copy link
Member

A more naive question: Have you considered writing unit test for it ? 😂
There is already indirect_call_can_be_added_to_pool_successfully

@Kailai-Wang
Copy link
Collaborator

I see, thanks. I missed the delegator on-chain logic part.

Kasper's comment makes sense too - I'm happy to have both : )

@felixfaisal felixfaisal enabled auto-merge (squash) March 18, 2024 04:13
@felixfaisal felixfaisal merged commit a9c2c99 into dev Mar 18, 2024
25 of 26 checks passed
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.

None yet

4 participants