Test/trex gateway #81
Open
+2,048
−26
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Open
julio4
requested changes
Jan 24, 2025
| @array![ | ||
| setup.trex_factory.contract_address.into(), | ||
| public_deployment_status.into(), | ||
| starknet::get_contract_address().into(), |
There was a problem hiding this comment.
TREXGateway needs to have the third arg starknet::get_contract_address()?
Is it not always the same, why not setting it as the caller in gateway contract?
Comment on lines
+189
to
+209
| fn test_should_set_new_status(status_u8: u8) { | ||
| let status = status_u8 % 2 == 0; | ||
| let (_, gateway) = setup_gateway(status); | ||
| let mut spy = spy_events(); | ||
|
|
||
| gateway.set_public_deployment_status(!status); | ||
| assert(gateway.get_public_deployment_status() == !status, 'Status didnt changed'); | ||
| spy | ||
| .assert_emitted( | ||
| @array![ | ||
| ( | ||
| gateway.contract_address, | ||
| TREXGateway::Event::PublicDeploymentStatusSet( | ||
| TREXGateway::PublicDeploymentStatusSet { | ||
| public_deployment_status: !status, | ||
| }, | ||
| ), | ||
| ), | ||
| ], | ||
| ); | ||
| } |
There was a problem hiding this comment.
Not sure if fuzz testing is the best here, especially because there's no bool support.
Suggested change
| fn test_should_set_new_status(status_u8: u8) { | |
| let status = status_u8 % 2 == 0; | |
| let (_, gateway) = setup_gateway(status); | |
| let mut spy = spy_events(); | |
| gateway.set_public_deployment_status(!status); | |
| assert(gateway.get_public_deployment_status() == !status, 'Status didnt changed'); | |
| spy | |
| .assert_emitted( | |
| @array![ | |
| ( | |
| gateway.contract_address, | |
| TREXGateway::Event::PublicDeploymentStatusSet( | |
| TREXGateway::PublicDeploymentStatusSet { | |
| public_deployment_status: !status, | |
| }, | |
| ), | |
| ), | |
| ], | |
| ); | |
| } | |
| fn test_should_set_new_status() { | |
| let (_, gateway) = setup_gateway(false); | |
| let mut spy = spy_events(); | |
| gateway.set_public_deployment_status(true); | |
| assert!(gateway.get_public_deployment_status(), 'Status didnt changed'); | |
| spy | |
| .assert_emitted( | |
| @array![ | |
| ( | |
| gateway.contract_address, | |
| TREXGateway::Event::PublicDeploymentStatusSet( | |
| TREXGateway::PublicDeploymentStatusSet { | |
| public_deployment_status: !status, | |
| }, | |
| ), | |
| ), | |
| ], | |
| ); | |
| // Optionally: test gateway.set_public_deployment_status(false) again | |
| } |
Comment on lines
+230
to
+241
| fn test_should_transfer_factory_ownership() { | ||
| let (setup, gateway) = setup_gateway(false); | ||
|
|
||
| let new_owner = starknet::contract_address_const::<'SOME_ADDRESS'>(); | ||
| gateway.transfer_factory_ownership(new_owner); | ||
| assert( | ||
| IOwnableDispatcher { contract_address: setup.trex_factory.contract_address } | ||
| .owner() == new_owner, | ||
| 'Ownership didnt transferred', | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Also test for OwnershipTransferred emit event
| ( | ||
| gateway.contract_address, | ||
| TREXGateway::Event::DeploymentFeeEnabled( | ||
| TREXGateway::DeploymentFeeEnabled { is_enabled: false }, |
There was a problem hiding this comment.
DeploymentFeeEnabled is really not a good event name if it's also emitted when disabling, but yeah let's keep same name as trex
Comment on lines
+331
to
+333
| let fee_collector = starknet::contract_address_const::<'FEE_COLLECTOR'>(); | ||
| let fee_token = setup.token.contract_address; | ||
| let fee = 100; |
There was a problem hiding this comment.
consider moving any const values in top of set_deployment_fee module (same for other modules)
Comment on lines
+443
to
+447
| let deployer = setup.accounts.alice.account.contract_address; | ||
| let mut spy = spy_events(); | ||
|
|
||
| start_cheat_caller_address( | ||
| gateway.contract_address, setup.accounts.token_agent.account.contract_address, |
There was a problem hiding this comment.
For clarity, suggest:
Suggested change
| let deployer = setup.accounts.alice.account.contract_address; | |
| let mut spy = spy_events(); | |
| start_cheat_caller_address( | |
| gateway.contract_address, setup.accounts.token_agent.account.contract_address, | |
| let deployer = setup.accounts.alice.account.contract_address; | |
| let agent = setup.accounts.token_agent.account.contract_address; | |
| let mut spy = spy_events(); | |
| start_cheat_caller_address( | |
| gateway.contract_address, agent, |
| }; | ||
| deployers.append(deployer); | ||
| // When batch has registered deployer should revert the whole batch | ||
| gateway.batch_add_deployer([deployer].span()); |
There was a problem hiding this comment.
Suggested change
| gateway.batch_add_deployer([deployer].span()); | |
| gateway.batch_add_deployer(deployers.span()); |
|
|
||
| #[test] | ||
| #[should_panic(expected: 'Deployer already exists')] | ||
| fn test_should_panic_when_called_by_agent_when_batch_includes_already_registered_deployer() { |
There was a problem hiding this comment.
Same, is it necessary? We have test_should_panic_when_called_by_owner_when_batch_includes_already_registered_deployer
| }; | ||
| deployers.append(deployer); | ||
| // When batch has registered deployer should revert the whole batch | ||
| gateway.batch_add_deployer([deployer].span()); |
There was a problem hiding this comment.
Suggested change
| gateway.batch_add_deployer([deployer].span()); | |
| gateway.batch_add_deployer(deployers.span()); |
| @setup, gateway, ref spy, token_details, not_deployer, 20_000, | ||
| ); | ||
| /// Check token transfer | ||
| assert(fee_token.balance_of(fee_collector) == 20_000, 'Fee not transferred'); |
There was a problem hiding this comment.
Also assert balance of not_deployer == 80_000
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.