Add test to showcase that job-creator would profit from liars #442
Conversation
WalkthroughThe changes introduce a new integration test in the file Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| f"Account A (job creator) ending difference: {ending_balance_A - starting_balance_A}." | ||
| ) | ||
| print( | ||
| f"Account B (attacker) ending difference: {ending_balance_B - starting_balance_B}." |
There was a problem hiding this comment.
Tests in CI are failing atm, but locally:
clear && pytest tests_integration_with_local_chain -k test_stealing_on_markets -vvv -s
| print(f"{api_keys_A.bet_from_address=}, {api_keys_B.bet_from_address=}") | ||
|
|
||
| # Update chain's state with a dummy block. | ||
| DebuggingContract().inc(api_keys_A, local_web3) |
There was a problem hiding this comment.
What's this for? Something to do with resetting the now time on the simulated chain?
There was a problem hiding this comment.
Yeah exactly, explained in detail here #437 (comment)
I abstracted this away to mint_new_block function to make it clear in the code.
tests_integration_with_local_chain/markets/omen/test_stealing_on_markets.py
Outdated
Show resolved
Hide resolved
tests_integration_with_local_chain/markets/omen/test_stealing_on_markets.py
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
tests_integration_with_local_chain/markets/omen/test_stealing_on_markets.py (3)
95-95: Typo in comment: "accout" should be "account".There's a typographical error in the comment. Correcting it improves readability.
Apply this diff to correct the typo:
- # Buy YES tokens from accout B (attacker) -- removing profit from any real agent that'd like to complete the job. + # Buy YES tokens from account B (attacker) -- removing profit from any real agent that'd like to complete the job.
113-113: Typo in comment: "loose" should be "lose".The word "loose" in the comment should be "lose" to convey the correct meaning.
Apply this diff to correct the typo:
- # Account A detects this and removes remaining liquidity, so the attacker is locked-in and will loose money unless he completes the job. + # Account A detects this and removes remaining liquidity, so the attacker is locked-in and will lose money unless he completes the job.
207-207: Typo in assertion message: "completors" should be "completers".The term "completors" is not standard. Replacing it with "completers" improves clarity.
This is already corrected in the previous diff.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests_integration_with_local_chain/markets/omen/test_stealing_on_markets.py (1 hunks)
🧰 Additional context used
🪛 Ruff
tests_integration_with_local_chain/markets/omen/test_stealing_on_markets.py
105-105: Local variable
balance_after_buying_Ais assigned to but never usedRemove assignment to unused variable
balance_after_buying_A(F841)
115-115: Local variable
balance_after_removing_funding_Bis assigned to but never usedRemove assignment to unused variable
balance_after_removing_funding_B(F841)
126-126: Local variable
e_buyingis assigned to but never usedRemove assignment to unused variable
e_buying(F841)
136-136: Local variable
e_sellingis assigned to but never usedRemove assignment to unused variable
e_selling(F841)
| assert ( | ||
| ending_balance_B < starting_balance_A | ||
| ), "Assumption was that B will loose the money he gambled by trying to steal from real job completors." |
There was a problem hiding this comment.
Fix assertion to compare B's ending balance with B's starting balance.
The assertion compares ending_balance_B with starting_balance_A, which is likely incorrect. It should compare ending_balance_B with starting_balance_B to verify that B has lost money.
Apply this diff to correct the assertion and fix the typo in the assertion message:
- assert (
- ending_balance_B < starting_balance_A
- ), "Assumption was that B will loose the money he gambled by trying to steal from real job completors."
+ assert (
+ ending_balance_B < starting_balance_B
+ ), "Assumption was that B will lose the money he gambled by trying to steal from real job completers."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert ( | |
| ending_balance_B < starting_balance_A | |
| ), "Assumption was that B will loose the money he gambled by trying to steal from real job completors." | |
| assert ( | |
| ending_balance_B < starting_balance_B | |
| ), "Assumption was that B will lose the money he gambled by trying to steal from real job completers." |
| ), "Balance after removing the liquidity should be higher than after market creation (because some liquidity can be withdrawn right away), but lower than before market creation (because some liquidity is now locked for the attacker's bet)" | ||
|
|
||
| # Buying or selling tokens after the liquidity is removed will fail. | ||
| with pytest.raises(Exception) as e_buying: |
There was a problem hiding this comment.
Unused exception variable e_buying.
The variable e_buying is assigned the exception but not used. If you don't need to inspect the exception, you can omit the as clause.
Apply this diff to remove the unused variable:
- with pytest.raises(Exception) as e_buying:
+ with pytest.raises(Exception):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with pytest.raises(Exception) as e_buying: | |
| with pytest.raises(Exception): |
🧰 Tools
🪛 Ruff
126-126: Local variable
e_buyingis assigned to but never usedRemove assignment to unused variable
e_buying(F841)
| balance_after_buying_A, balance_after_buying_B = ( | ||
| get_balances(api_keys_A.bet_from_address, local_web3).total, | ||
| get_balances(api_keys_B.bet_from_address, local_web3).total, | ||
| ) |
There was a problem hiding this comment.
Remove unused variable balance_after_buying_A.
The variable balance_after_buying_A is assigned but never used. Removing it cleans up the code and avoids confusion.
Apply this diff to remove the unused variable:
- balance_after_buying_A, balance_after_buying_B = (
- get_balances(api_keys_A.bet_from_address, local_web3).total,
- get_balances(api_keys_B.bet_from_address, local_web3).total,
- )
+ balance_after_buying_B = get_balances(api_keys_B.bet_from_address, local_web3).total📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| balance_after_buying_A, balance_after_buying_B = ( | |
| get_balances(api_keys_A.bet_from_address, local_web3).total, | |
| get_balances(api_keys_B.bet_from_address, local_web3).total, | |
| ) | |
| balance_after_buying_B = get_balances(api_keys_B.bet_from_address, local_web3).total |
🧰 Tools
🪛 Ruff
105-105: Local variable
balance_after_buying_Ais assigned to but never usedRemove assignment to unused variable
balance_after_buying_A(F841)
| web3=local_web3, | ||
| ) | ||
| sell_yes_for_b = xdai_type(1) | ||
| with pytest.raises(Exception) as e_selling: |
There was a problem hiding this comment.
Unused exception variable e_selling.
Similarly, e_selling is assigned but not used. You can remove it if it's not needed.
Apply this diff to remove the unused variable:
- with pytest.raises(Exception) as e_selling:
+ with pytest.raises(Exception):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with pytest.raises(Exception) as e_selling: | |
| with pytest.raises(Exception): |
🧰 Tools
🪛 Ruff
136-136: Local variable
e_sellingis assigned to but never usedRemove assignment to unused variable
e_selling(F841)
| balance_after_removing_funding_A, balance_after_removing_funding_B = ( | ||
| get_balances(api_keys_A.bet_from_address, local_web3).total, | ||
| get_balances(api_keys_B.bet_from_address, local_web3).total, | ||
| ) |
There was a problem hiding this comment.
Remove unused variable balance_after_removing_funding_B.
The variable balance_after_removing_funding_B is assigned but never used. Removing it tidies up the code.
Apply this diff to remove the unused variable:
- balance_after_removing_funding_A, balance_after_removing_funding_B = (
- get_balances(api_keys_A.bet_from_address, local_web3).total,
- get_balances(api_keys_B.bet_from_address, local_web3).total,
- )
+ balance_after_removing_funding_A = get_balances(api_keys_A.bet_from_address, local_web3).total📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| balance_after_removing_funding_A, balance_after_removing_funding_B = ( | |
| get_balances(api_keys_A.bet_from_address, local_web3).total, | |
| get_balances(api_keys_B.bet_from_address, local_web3).total, | |
| ) | |
| balance_after_removing_funding_A = get_balances(api_keys_A.bet_from_address, local_web3).total |
🧰 Tools
🪛 Ruff
115-115: Local variable
balance_after_removing_funding_Bis assigned to but never usedRemove assignment to unused variable
balance_after_removing_funding_B(F841)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
tests_integration_with_local_chain/markets/omen/test_stealing_on_markets.py (4)
95-95: Fix typo in comment: 'accout B' should be 'account B'There's a typo in the comment at line 95: 'accout B' should be 'account B'.
Apply this diff to correct the typo:
- # Buy YES tokens from accout B (attacker) -- removing profit from any real agent that'd like to complete the job. + # Buy YES tokens from account B (attacker) -- removing profit from any real agent that'd like to complete the job.
108-108: Correct typo in assertion messageThe assertion message at line 108 contains a typo: 'should have be lowered' should be 'should have been lowered'.
Apply this diff to fix the typo:
- ), "Balance of B should have be lowered from betting" + ), "Balance of B should have been lowered from betting"
110-110: Correct typo in comment: 'loose' should be 'lose'The word 'loose' in the comment at line 110 should be 'lose'.
Apply this diff to correct the spelling:
- # Account A detects this and removes remaining liquidity, so the attacker is locked-in and will loose money unless he completes the job. + # Account A detects this and removes remaining liquidity, so the attacker is locked-in and will lose money unless he completes the job.
172-172: Fix typo in comment: 'manualy' should be 'manually'There's a typo in the comment at line 172: 'manualy' should be 'manually'.
Apply this diff to correct the typo:
- # Note: Usually we just take all positions from subgraph and redeem them, here we manualy redeem the ones we should have now. + # Note: Usually we just take all positions from subgraph and redeem them, here we manually redeem the ones we should have now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests_integration_with_local_chain/markets/omen/test_stealing_on_markets.py (1 hunks)
🧰 Additional context used
🪛 Ruff
tests_integration_with_local_chain/markets/omen/test_stealing_on_markets.py
122-122:
pytest.raises(Exception)should be considered evil(B017)
132-132:
pytest.raises(Exception)should be considered evil(B017)
| ), "Balance after failed trading should be the same as after buying of tokens in the beginning, because nothing should have happened." | ||
|
|
||
| # Wait for market's closing time | ||
| time.sleep(close_in * 1.1) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider mocking time to avoid using time.sleep in tests
Using time.sleep in tests can lead to longer test execution times and potential flakiness. Consider mocking the time or adjusting the contract state to avoid waiting in real time during tests.
Also applies to: 163-163
| omen_remove_fund_market_tx(api_keys_A, agent_market, shares=None, web3=local_web3) | ||
| balance_after_removing_funding_A = get_balances( | ||
| api_keys_A.bet_from_address, local_web3 | ||
| ).total | ||
| assert ( | ||
| balance_after_market_creation_A | ||
| < balance_after_removing_funding_A | ||
| < starting_balance_A | ||
| ), "Balance after removing the liquidity should be higher than after market creation (because some liquidity can be withdrawn right away), but lower than before market creation (because some liquidity is now locked for the attacker's bet)" | ||
|
|
||
| # Buying or selling tokens after the liquidity is removed will fail. | ||
| with pytest.raises(Exception): | ||
| binary_omen_buy_outcome_tx( | ||
| api_keys_B, | ||
| buy_yes_for_b, | ||
| agent_market, | ||
| binary_outcome=True, | ||
| auto_deposit=True, | ||
| web3=local_web3, | ||
| ) | ||
| sell_yes_for_b = xdai_type(1) | ||
| with pytest.raises(Exception): | ||
| binary_omen_sell_outcome_tx( | ||
| api_keys_B, | ||
| sell_yes_for_b, | ||
| agent_market, | ||
| binary_outcome=True, | ||
| auto_withdraw=True, | ||
| web3=local_web3, | ||
| ) | ||
| balance_after_failed_trading_B = get_balances( | ||
| api_keys_B.bet_from_address, local_web3 | ||
| ).total | ||
| assert ( | ||
| balance_after_failed_trading_B == balance_after_buying_B | ||
| ), "Balance after failed trading should be the same as after buying of tokens in the beginning, because nothing should have happened." | ||
|
|
||
| # Wait for market's closing time | ||
| time.sleep(close_in * 1.1) | ||
| # Do a dummy block again, so the time in the contract is updated and it knows it's opened already. | ||
| mint_new_block(api_keys_A, local_web3) | ||
|
|
||
| # Submit answer on reality. | ||
| omen_submit_answer_market_tx( | ||
| api_keys_A, | ||
| omen_market, | ||
| Resolution.NO, | ||
| bond=xdai_type(0.001), | ||
| web3=local_web3, | ||
| ) | ||
|
|
||
| # Wait for the finalization. | ||
| time.sleep(finalization_wait_time_seconds * 1.1) | ||
| # Update the time in the chain again. | ||
| mint_new_block(api_keys_A, local_web3) | ||
|
|
||
| # Resolve the market. | ||
| omen_resolve_market_tx(api_keys_A, omen_market, local_web3) | ||
|
|
||
| # Redeem positions from both accounts. | ||
| # Note: Usually we just take all positions from subgraph and redeem them, here we manualy redeem the ones we should have now. | ||
| conditional_token_contract = OmenConditionalTokenContract() | ||
| condition_event = check_not_none( | ||
| created_market.condition_event, | ||
| "Should not be None here as this was a freshly created market.", | ||
| ) | ||
| conditional_token_contract.redeemPositions( | ||
| api_keys=api_keys_A, | ||
| collateral_token_address=agent_market.collateral_token_contract_address_checksummed, | ||
| condition_id=condition_event.conditionId, | ||
| index_sets=omen_market.condition.index_sets, | ||
| web3=local_web3, | ||
| ) | ||
| conditional_token_contract.redeemPositions( | ||
| api_keys=api_keys_B, | ||
| collateral_token_address=agent_market.collateral_token_contract_address_checksummed, | ||
| condition_id=condition_event.conditionId, | ||
| index_sets=omen_market.condition.index_sets, | ||
| web3=local_web3, | ||
| ) | ||
|
|
||
| # Check who is the winner in the end. | ||
| ending_balance_A, ending_balance_B = ( | ||
| get_balances(api_keys_A.bet_from_address, local_web3).total, | ||
| get_balances(api_keys_B.bet_from_address, local_web3).total, | ||
| ) | ||
|
|
||
| assert ( | ||
| ending_balance_A > starting_balance_A | ||
| ), "Assumption was that A will receive B's money." | ||
| assert ( | ||
| ending_balance_B < starting_balance_A | ||
| ), "Assumption was that B will loose the money he gambled by trying to steal from real job completors." | ||
|
|
||
| print( | ||
| f"Account A (job creator) ending difference: {ending_balance_A - starting_balance_A}." | ||
| ) | ||
| print( | ||
| f"Account B (attacker) ending difference: {ending_balance_B - starting_balance_B}." | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor test into smaller functions for better readability
The test function test_stealing_on_markets is quite long, which can make it harder to read and maintain. Consider refactoring it into smaller helper functions to improve readability and maintainability.
🧰 Tools
🪛 Ruff
122-122:
pytest.raises(Exception)should be considered evil(B017)
132-132:
pytest.raises(Exception)should be considered evil(B017)
| web3=local_web3, | ||
| ) | ||
| sell_yes_for_b = xdai_type(1) | ||
| with pytest.raises(Exception): |
There was a problem hiding this comment.
Specify the expected exception type in pytest.raises
Using pytest.raises(Exception) is too general and might catch unexpected exceptions. Please specify the exact exception type that is expected.
Apply this diff:
- with pytest.raises(Exception):
+ with pytest.raises(ExpectedExceptionType):Replace ExpectedExceptionType with the appropriate exception.
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Ruff
132-132:
pytest.raises(Exception)should be considered evil(B017)
| ), "Balance after removing the liquidity should be higher than after market creation (because some liquidity can be withdrawn right away), but lower than before market creation (because some liquidity is now locked for the attacker's bet)" | ||
|
|
||
| # Buying or selling tokens after the liquidity is removed will fail. | ||
| with pytest.raises(Exception): |
There was a problem hiding this comment.
Specify the expected exception type in pytest.raises
Using pytest.raises(Exception) is too broad and may catch unintended exceptions. It's recommended to specify the exact exception type that is expected to be raised.
Apply this diff to specify the expected exception type:
- with pytest.raises(Exception):
+ with pytest.raises(ExpectedExceptionType):Replace ExpectedExceptionType with the specific exception that should be raised in this context.
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Ruff
122-122:
pytest.raises(Exception)should be considered evil(B017)
No description provided.