Skip to content
This repository has been archived by the owner on Aug 2, 2024. It is now read-only.

test(rust-rpc-test): have a contract to declare which is not already declared #1110

Closed
tdelabro opened this issue Sep 18, 2023 · 19 comments · Fixed by #1286
Closed

test(rust-rpc-test): have a contract to declare which is not already declared #1110

tdelabro opened this issue Sep 18, 2023 · 19 comments · Fixed by #1286
Assignees
Labels
backlog Ready to be picked enhancement New feature or request testing

Comments

@tdelabro
Copy link
Collaborator

during the genesis phase of the sharingan test-chains, we populate the state with a few contracts already declared and deployed.

One of them is ERC20.json. Due to some shenanigans, it was declared with another arbitrary class hash, making it possible for users to re-declare it a second time, this time with it's real class hash.

Some tests used this contract in order to test the declare workflow starknet-rpc-test::get_transaction_receipt:: work_with_declare_transaction is a survivant of this era that we are ignoring for now.

One should activate this test again by making it declare another contract class, one which is not already present in the storage

@tdelabro tdelabro added enhancement New feature or request testing backlog Ready to be picked labels Sep 18, 2023
@tdelabro tdelabro added this to Madara Sep 18, 2023
@tdelabro tdelabro moved this to 📋 Backlog in Madara Sep 18, 2023
@tdelabro tdelabro moved this from 📋 Backlog to 🔖 Ready in Madara Sep 18, 2023
@NaitikChaudhary
Copy link

Can I take this one?

@apoorvsadana
Copy link
Collaborator

Assigned to you!

@tdelabro tdelabro moved this from 🔖 Ready to 🏗 In progress in Madara Sep 28, 2023
@tdelabro
Copy link
Collaborator Author

Hey, all good?
We can unassign you if you can't find time. No big deal

@NaitikChaudhary
Copy link

NaitikChaudhary commented Oct 12, 2023 via email

@tdelabro
Copy link
Collaborator Author

Yeah sure.
2 ways you can do it.

  • group all the tests regarding declare in one single tests and do all the checks after a single declare
  • create n contracts with one internal constant ranging from 0 to n. Then use one of them in each test that need to call declare

@tdelabro
Copy link
Collaborator Author

Also, check #1145 I flag all tests using declare as ignored because they were all redeploying the same contract.
You will have to un-ignore them in your PR

@tdelabro
Copy link
Collaborator Author

@NaitikChaudhary All good?

@tdelabro
Copy link
Collaborator Author

@NaitikChaudhary Will you deliver this one or can I unassign it from you?

@apoorvsadana
Copy link
Collaborator

I don't think he's working on it. Unassigned him.

@tdelabro tdelabro moved this from 🏗 In progress to 🔖 Ready in Madara Nov 17, 2023
@edisontim
Copy link
Contributor

Hey, can I take this :) ?

@tdelabro
Copy link
Collaborator Author

@edisontim everything for my fellow 42 alumni

@tdelabro tdelabro moved this from 🔖 Ready to 🏗 In progress in Madara Nov 18, 2023
@edisontim
Copy link
Contributor

Thanks! On it :)

@edisontim
Copy link
Contributor

Yeah sure. 2 ways you can do it.

  • group all the tests regarding declare in one single tests and do all the checks after a single declare
  • create n contracts with one internal constant ranging from 0 to n. Then use one of them in each test that need to call declare

Contract being declared in starknet-rpc-test::get_transaction_receipt:: work_with_declare_transaction is Counter.json, so this is the contract that can't be redeclared multiple times right?

I think I'll go first the first solution, this seems the cleanest now. However, if more declare tests need to happen this could get messy

@tdelabro
Copy link
Collaborator Author

Yes that is the one.

I think first option is okay. Yes you will have to organize and comment your code correctly so it don't get nightmarish

@edisontim
Copy link
Contributor

Had a look at a global variable shared between all tokio tests or a single use init function that launched before tests but wasn't able to find anything viable :/

@tdelabro
Copy link
Collaborator Author

Wdym?
You have to run your node manually and then launch the test.

@edisontim
Copy link
Contributor

I mean having a setup function before all rpc-tests that would declare the contract once for all tests and make available a global declare_tx variable, but it seems like with async tests that's impossible

@tdelabro
Copy link
Collaborator Author

tdelabro commented Nov 22, 2023

Yep, there is no init logic in cargo test. I would not rely on this behavior.
What you can do is create a function declare_if_not_already() { if already_declared {} else { declare() } } and call it in all your tests that need this contract deployed. A bit hacky but it solve you problem

@edisontim
Copy link
Contributor

Yeah I'm having a look at that now, it's cleaner than having n contracts declared imo

@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Madara Nov 29, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backlog Ready to be picked enhancement New feature or request testing
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants