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

added pallet test for InvokeTransaction::V0 #1608

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

raizo07
Copy link

@raizo07 raizo07 commented May 18, 2024

Pull Request type

Please add the labels corresponding to the type of changes your PR introduces:

  • Testing
  • Feature

What is the current behavior?

All the transaction tests in are InvokeTransaction::V0

Resolves: missing V0 tests.

What is the new behavior?

  • We have a possibility to check V0 invokeTransactions.

Does this introduce a breaking change?

  • No

Other information

@raizo07
Copy link
Author

raizo07 commented May 18, 2024

@tdelabro I've opened a PR here, I think you should be able to review here. Thank you.

@EvolveArt
Copy link
Collaborator

thanks for this pr @raizo07, I think the issue was about declare v0 tho. Also in this PR you modify an existing test instead of adding a new one. Please also update changelog.

@raizo07
Copy link
Author

raizo07 commented May 21, 2024

thanks for this pr @raizo07, I think the issue was about declare v0 tho. Also in this PR you modify an existing test instead of adding a new one. Please also update changelog.

@EvolveArt Thank you for your comment. But from this link #1603 (comment) You can see that it is a different task I was asked to work on, of which I added the test. I'm open to further reviews.

crates/pallets/starknet/src/tests/invoke_tx.rs Outdated Show resolved Hide resolved
crates/pallets/starknet/src/tests/invoke_tx.rs Outdated Show resolved Hide resolved
crates/pallets/starknet/src/tests/invoke_tx.rs Outdated Show resolved Hide resolved
crates/pallets/starknet/src/tests/invoke_tx.rs Outdated Show resolved Hide resolved
@tdelabro
Copy link
Collaborator

@raizo07, still working on it?

@raizo07
Copy link
Author

raizo07 commented May 30, 2024

@raizo07, still working on it?

Yes, I'll be sending the PR today.

new_test_ext::<MockRuntime>().execute_with(|| {
basic_test_setup(2);

let mut transaction = get_invoke_dummy(Starknet::chain_id(), NONCE_ZERO);
Copy link
Collaborator

Choose a reason for hiding this comment

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

get_invoke_dummy will always return a TxV1. That what it is coded to do

Copy link
Collaborator

Choose a reason for hiding this comment

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

so you never enter if let starknet_api::transaction::InvokeTransaction::V0(tx)
and therefore you are execution a tx v1 here: assert_ok!(Starknet::invoke(RuntimeOrigin::none(), transaction));

Copy link
Author

Choose a reason for hiding this comment

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

I'm resolving it now

@tdelabro
Copy link
Collaborator

tdelabro commented Jun 3, 2024

@raizo07 this time the tests don't even compile.
run cargo test on you computer

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

3 participants