Add a new contract used for debugging #437
Conversation
WalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
| def test_now(local_web3: Web3, test_keys: APIKeys) -> None: | ||
| # we need to mint a new block to update timestamp | ||
| DebuggingContract().inc(test_keys, local_web3) | ||
| allowed_difference = 10 # seconds | ||
| chain_timestamp = DebuggingContract().getNow(local_web3) | ||
| utc_timestamp = int(utcnow().timestamp()) | ||
| assert ( | ||
| abs(chain_timestamp - utc_timestamp) <= allowed_difference | ||
| ), f"chain_timestamp and utc_timestamp differ by more than {allowed_difference} seconds: {chain_timestamp=} {utc_timestamp=}" | ||
|
|
||
|
|
||
| def test_now_failed(local_web3: Web3, test_keys: APIKeys) -> None: | ||
| # Sleep a little to let the local chain go out of sync without updating the block | ||
| time.sleep(5) | ||
| allowed_difference = 10 # seconds | ||
| chain_timestamp = DebuggingContract().getNow(local_web3) | ||
| utc_timestamp = int(utcnow().timestamp()) | ||
| assert ( | ||
| abs(chain_timestamp - utc_timestamp) >= allowed_difference | ||
| ), f"without minting a new block, timestamps should differ by more than {allowed_difference} seconds: {chain_timestamp=} {utc_timestamp=}" | ||
|
|
||
|
|
||
| def test_now_datetime(local_web3: Web3, test_keys: APIKeys) -> None: | ||
| # we need to mint a new block to update timestamp | ||
| DebuggingContract().inc(test_keys, local_web3) | ||
| allowed_difference = 10 # seconds | ||
| chain_datetime = DebuggingContract().get_now(local_web3) | ||
| utc_datetime = utcnow() | ||
| actual_difference = abs(chain_datetime - utc_datetime) | ||
| assert actual_difference <= timedelta( | ||
| seconds=allowed_difference | ||
| ), f"chain_datetime and utc_datetime differ by more than {allowed_difference} seconds: {chain_datetime=} {utc_datetime=} {actual_difference=}" |
There was a problem hiding this comment.
Those tests aren't very clear to me 🤔
By default, Anvil creates a new block every time a new transaction is completed.
https://book.getfoundry.sh/reference/anvil/#mining-modes
Additionally, DebuggingContract simply outputs block.timestamp, which can be more easily fetched (without a smart contract) via local_web3.eth.block_number.
So I'm not sure what exactly is the goal of this test - could you please clarify?
There was a problem hiding this comment.
When debugging something, it's good to be as close to the real scenario as possible. Reality's contract isn't using local_web3.eth.block_number, it's using now in its functions.
Compare test_now and test_now_failed.
These tests verify my assumption that if you want now in the contract to be up-to-date, you have to mint a new block (using the DebuggingContract().inc(test_keys, local_web3).
There was a problem hiding this comment.
These tests verify my assumption that if you want now in the contract to be up-to-date, you have to mint a new block
We are talking about different things - now reflects the block timestamp when the transaction was executed. On Anvil, since each block has one transaction, the block is quickly confirmed hence timestamps don't differ much. On Gnosis mainnet, block confirmation takes a little longer (approx 6s), hence now would return an outdated value.
you have to mint a new block (using the DebuggingContract().inc(test_keys, local_web3).
That's not entirely true. You can also achieve the exact same thing using the web3 provider.
previous_block_timestamp = local_web3.eth.get_block('latest').timestamp
local_web3.manager.request_blocking(RPC.evm_mine, [previous_block_timestamp + 1]) # next block timestamp >= prev block timestamp
There was a problem hiding this comment.
This is what I was talking about on standup. Imagine this:
- Anvil creates local_chain, it's timestamp (
now) is updated - You create a new question, there's a new transaction because of it, so timestamp (
now) is updated again - You do
time.sleep(X)whereXis the time of seconds when the question should open - You want to answer this question, the first thing that contracts does is checking if
opening_timestamp >= now, however, because there was no transaction between (3) and (4),nowin your local chain is still old, and so the answering fails with revert error.
So between (3) and (4), you need to do a dummy transaction that will cause the local chain to be updated. Then (4) works because now in the contract is updated.
That's not entirely true. You can also achieve the exact same thing using the web3 provider.
That's interesting. What is RPC in your code? But executing a dummy transaction DebuggingContract().inc(test_keys, local_web3) still feels more real-world-like.
There was a problem hiding this comment.
I agree with your logic - the trade-off here is, to implement your test, you can either deploy a new contract OR use standard web3py methods. The less complex option is probably the latter.
What is RPC in your code?
from web3._utils.rpc_abi import RPC
There was a problem hiding this comment.
It's already deployed, so not a big deal. Do you see any potential issues arising from the current implementation? If not, then I'd like to keep it the current way.
There was a problem hiding this comment.
Yes, please see comment -> gnosis/labs-contracts#9 (comment)
As far as I understood the situation (please correct me if I'm wrong), the contract is not needed in favor of web3py methods. Surely the contract has already been deployed, but if it's not needed then we should remove it.
Removing it implies on re-writing 2 tests on this PR with the method I mentioned earlier RPC.evm_mine, so it's also not a big deal.
There was a problem hiding this comment.
Yes, please see comment -> gnosis/labs-contracts#9 (comment)
You don't mention any potential issues in that comment. What problems do you reckon?
I'm arguing for using the current way because it resembles the real production scenario more.
I'm going to move DebuggingContract().inc(test_keys, local_web3) to a function to make it more modular, but otherwise, what problem do you see here?
There was a problem hiding this comment.
From my comment
Everything that is achieved calling the getNow function can be achieved calling web3.eth.get_block('latest').timestamp
From the Realitio contract (I assume this is what you mean by real production scenaio?)(https://gnosisscan.io/address/0x79e32aE03fb27B07C89c0c568F80287C01ca2E57#code#L222)
require(finalize_ts == UNANSWERED || finalize_ts > uint32(now), "finalization deadline must not have passed");
I don't see any potential issues with the contract implementation. What I'm saying is, you can either
a. use the current PR and new contract to mine a new block on a local chain and also test a native EVM function (block.timestamp) OR
b. Use standard functions from web3py and RPC method auto_mine for accomplishing the exact same thing, without an extra contract. You can also test Realitio contract using their ABI (it's verified).
There was a problem hiding this comment.
What I'm saying is, you can either
Yeah, and I said a few comments above that I preferred the a., so the question from my side was if you see any issues with that implementation.
Because you don't, is that still, for some reason, a real blocker for you, or can you approve so I can go ahead and merge all the chained PRs?
If it bugs you too much I'll change it 😄 If not it makes more sense like this to me.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
tests/utils.py (1)
11-16: LGTM: Function implementation is correct. Minor typo in docstring.The
mint_new_blockfunction is well-implemented and serves its purpose of minting a new block and updating the chain's timestamp. The function signature, type annotations, and use ofDebuggingContractare all correct.There's a small typo in the docstring:
- Useful for tests that debends on chain's timestamp, this will update it. + Useful for tests that depends on chain's timestamp, this will update it.tests_integration_with_local_chain/markets/omen/test_local_chain.py (5)
Line range hint
1-23: Remove unused importtimedelta.The
timedeltaimport fromdatetimeis not used in the visible code. Consider removing it to keep the imports clean.Apply this diff to remove the unused import:
-from datetime import timedelta
104-112: LGTM! Consider extracting the allowed difference as a constant.The implementation correctly tests the synchronization of the blockchain timestamp with UTC time after minting a new block. This aligns with the explanation provided in the past review comments.
To improve maintainability, consider extracting the
allowed_differenceas a module-level constant, as it's used in multiple test functions.Apply this diff to extract the constant:
+ALLOWED_TIMESTAMP_DIFFERENCE = 15 # seconds def test_now(local_web3: Web3, test_keys: APIKeys) -> None: # we need to mint a new block to update timestamp mint_new_block(test_keys, local_web3) - allowed_difference = 15 # seconds chain_timestamp = DebuggingContract().getNow(local_web3) utc_timestamp = int(utcnow().timestamp()) assert ( - abs(chain_timestamp - utc_timestamp) <= allowed_difference - ), f"chain_timestamp and utc_timestamp differ by more than {allowed_difference} seconds: {chain_timestamp=} {utc_timestamp=}" + abs(chain_timestamp - utc_timestamp) <= ALLOWED_TIMESTAMP_DIFFERENCE + ), f"chain_timestamp and utc_timestamp differ by more than {ALLOWED_TIMESTAMP_DIFFERENCE} seconds: {chain_timestamp=} {utc_timestamp=}"
115-123: LGTM! Consider using the same allowed difference constant.The implementation correctly tests the scenario where the blockchain timestamp diverges from UTC time when no new block is minted. This complements the
test_nowfunction and aligns with the explanation in the past review comments.For consistency, consider using the same
ALLOWED_TIMESTAMP_DIFFERENCEconstant suggested earlier, but with a smaller value for this test.Apply this diff to use the constant:
+ALLOWED_TIMESTAMP_DIFFERENCE_SMALL = 5 # seconds def test_now_failed(local_web3: Web3, test_keys: APIKeys) -> None: # Sleep a little to let the local chain go out of sync without updating the block time.sleep(5) - allowed_difference = 5 # seconds chain_timestamp = DebuggingContract().getNow(local_web3) utc_timestamp = int(utcnow().timestamp()) assert ( - abs(chain_timestamp - utc_timestamp) >= allowed_difference - ), f"without minting a new block, timestamps should differ by more than {allowed_difference} seconds: {chain_timestamp=} {utc_timestamp=}" + abs(chain_timestamp - utc_timestamp) >= ALLOWED_TIMESTAMP_DIFFERENCE_SMALL + ), f"without minting a new block, timestamps should differ by more than {ALLOWED_TIMESTAMP_DIFFERENCE_SMALL} seconds: {chain_timestamp=} {utc_timestamp=}"
126-135: LGTM! Consider improvements for consistency and clarity.The implementation correctly tests the synchronization of blockchain and UTC time using datetime objects. This provides a valuable addition to the timestamp-based tests.
Consider the following improvements:
- Use the
ALLOWED_TIMESTAMP_DIFFERENCEconstant for consistency.- Clarify the difference between
getNow()andget_now()methods ofDebuggingContract.- Use a more descriptive variable name for the time difference.
Apply this diff to implement the suggested improvements:
def test_now_datetime(local_web3: Web3, test_keys: APIKeys) -> None: # we need to mint a new block to update timestamp mint_new_block(test_keys, local_web3) - allowed_difference = 15 # seconds chain_datetime = DebuggingContract().get_now(local_web3) utc_datetime = utcnow() - actual_difference = (utc_datetime - chain_datetime).total_seconds() + time_difference_seconds = abs((utc_datetime - chain_datetime).total_seconds()) assert ( - actual_difference <= allowed_difference - ), f"chain_datetime and utc_datetime differ by more than {allowed_difference} seconds: {chain_datetime=} {utc_datetime=} {actual_difference=}" + time_difference_seconds <= ALLOWED_TIMESTAMP_DIFFERENCE + ), f"chain_datetime and utc_datetime differ by more than {ALLOWED_TIMESTAMP_DIFFERENCE} seconds: {chain_datetime=} {utc_datetime=} {time_difference_seconds=}"Also, consider adding a comment to explain the difference between
getNow()andget_now()methods ofDebuggingContract.
Line range hint
1-135: Overall implementation looks good, addressing past concerns.The current implementation effectively demonstrates the behavior of blockchain timestamps and the need for minting new blocks to keep them synchronized. It aligns with your preference for using a contract to resemble real-world scenarios, as discussed in the past review comments.
While alternative methods using web3py were suggested, the current implementation using
DebuggingContractis valid and meets the testing requirements. It provides a clear and practical way to verify timestamp behavior in a local blockchain environment.To further improve the implementation, consider:
- Adding a brief comment explaining why
DebuggingContractis used instead of web3py methods, to provide context for future readers.- Ensuring that
DebuggingContractis well-documented, especially the difference betweengetNow()andget_now()methods.tests_integration_with_local_chain/markets/omen/test_omen.py (2)
Line range hint
377-381: Add error handling for thewithdrawoperationThe
WrappedxDaiContract().withdraw()function call is an external operation that can potentially fail due to network issues or insufficient gas. Consider adding error handling to catch exceptions and ensure the test fails gracefully if the withdrawal is unsuccessful.Apply this diff to add error handling:
if initial_balances.wxdai > 0: - WrappedxDaiContract().withdraw( + try: + WrappedxDaiContract().withdraw( api_keys=test_keys, amount_wei=xdai_to_wei(initial_balances.wxdai), web3=local_web3, + ) + except Exception as e: + pytest.fail(f"Withdrawal failed: {e}")
Line range hint
390-396: Add verification after placing the betAfter calling
market.place_bet(), consider adding assertions to verify that the bet was placed successfully. This could include checking the user's position in the market to ensure it reflects the new bet.You can add the following code after the
place_betcall:# Verify that the user's position has been updated new_position = market.get_token_balance( user_id=test_keys.bet_from_address, outcome=True, web3=local_web3, ) assert new_position.amount >= bet_amount.amount, "Bet amount not reflected in position balance"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
prediction_market_agent_tooling/abis/debuggingcontract.abi.jsonis excluded by!**/*.json
📒 Files selected for processing (4)
- prediction_market_agent_tooling/tools/contract.py (3 hunks)
- tests/utils.py (1 hunks)
- tests_integration_with_local_chain/markets/omen/test_local_chain.py (3 hunks)
- tests_integration_with_local_chain/markets/omen/test_omen.py (1 hunks)
🧰 Additional context used
🪛 Ruff
tests_integration_with_local_chain/markets/omen/test_local_chain.py
2-2:
datetime.timedeltaimported but unusedRemove unused import:
datetime.timedelta(F401)
🔇 Additional comments (8)
tests/utils.py (1)
3-6: LGTM: Import statements are correct and necessary.The new import statements are correctly placed and are required for the newly added
mint_new_blockfunction. Good job on keeping the imports organized and relevant.tests_integration_with_local_chain/markets/omen/test_omen.py (1)
385-385: Good use ofnp.isclosefor floating-point comparisonUsing
np.iscloseto compareinitial_balances.wxdaiwithxdai_type(0)improves the reliability of the test by properly handling floating-point precision issues when checking for zero balances.prediction_market_agent_tooling/tools/contract.py (6)
6-6: Imports for datetime utilitiesThe addition of
datetimeandpytzimports is appropriate for handling timezone-aware datetime objects.Also applies to: 8-8
26-30: Importing utility functionsImporting
DatetimeWithTimezone,add_utc_timezone_validator, andshould_not_happenfromtools.utilsensures these utilities are available for the new methods.
432-442: Creation ofDebuggingContractclassThe new
DebuggingContractclass correctly extendsContractOnGnosisChainand sets up the ABI and address appropriately. This addition aligns with the PR objective of adding a new contract for debugging.
444-453: Implementation ofgetNowmethodThe
getNowmethod correctly calls thegetNowfunction on the contract and returns the timestamp as an integer.
454-461: Conversion to timezone-aware datetime inget_nowmethodThe
get_nowmethod appropriately converts the timestamp fromgetNowto aDatetimeWithTimezoneobject with UTC timezone. Utilizingadd_utc_timezone_validatorensures the datetime object is correctly validated.
462-472: Implementation ofincmethodThe
incmethod correctly implements the invocation of theincfunction on the contract, allowing state changes through thesendmethod.
| address: ChecksumAddress = Web3.to_checksum_address( | ||
| "0x5Aa82E068aE6a6a1C26c42E5a59520a74Cdb8998" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider parameterizing the contract address
The contract address is hard-coded in the DebuggingContract class. For better flexibility and maintainability, consider passing the address as a parameter or reading it from a configuration file. This will make it easier to update the address if it changes in the future.
No description provided.