Add popup with unprocessed messages #603
Conversation
kongzii
commented
Dec 13, 2024
kongzii
commented
WalkthroughThe pull request introduces modifications to two files: Changes
Possibly related PRs
Suggested reviewers
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 (
|
| transactions = blockchain_transaction_fetcher().fetch_unseen_transactions( | ||
| nft_agent.wallet_address | ||
| ) |
There was a problem hiding this comment.
If I click 2x on the button, does it trigger fetch_unseen_transactions 2x?
spice is caching results, but still I would suggest placing the transactions inside a st.session_state, updating that through st.fragment and just reading from that inside the popover. Wdyt?
There was a problem hiding this comment.
This whole thing is in the fragment already:
I don't see how saving it manually to state would help?
As for the button, I tested it and code inside of with block is executed regardless of clicking the button:
There was a problem hiding this comment.
Ow, understood - so button simply toggles the disabled property and shows stuff, but doesn't trigger any fetching logic. Fine.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
prediction_market_agent/db/blockchain_transaction_fetcher.py (1)
Line range hint
44-70: Improve performance by avoiding row-wise iteration over the DataFrameUsing
df.iter_rows(named=True)for converting DataFrame rows toBlockchainMessageobjects can be inefficient for large datasets. Polars supports vectorized operations, which are more performant.Consider using
df.applyor vectorized methods to convert the DataFrame to a list ofBlockchainMessageobjects without explicit iteration:return [ - self.blockchain_message_from_dune_df_row(consumer_address, x) - for x in df.iter_rows(named=True) + self.blockchain_message_from_dune_df_row(consumer_address, row) + for row in df.to_dicts() ]This approach leverages Polars' efficient data handling capabilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py(4 hunks)prediction_market_agent/db/blockchain_transaction_fetcher.py(4 hunks)
🔇 Additional comments (1)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py (1)
205-207: Optimize transaction fetching to avoid redundant API calls
Repeatedly calling fetch_unseen_transactions inside the popover may lead to unnecessary API requests if the popover is opened multiple times. This can affect performance and user experience.
Consider storing the transactions in st.session_state or updating them through a st.fragment, then reading from that inside the popover. This approach reduces redundant API calls and improves efficiency.
| def blockchain_message_from_dune_df_row( | ||
| self, consumer_address: ChecksumAddress, x: dict[str, Any] | ||
| ) -> BlockchainMessage: | ||
| return BlockchainMessage( | ||
| consumer_address=consumer_address, | ||
| transaction_hash=x["hash"], | ||
| value_wei=str(x["value"]), | ||
| block=str(x["block_number"]), | ||
| sender_address=x["from"], | ||
| data_field=self.unzip_message_else_do_nothing(x["data"]), | ||
| ) |
There was a problem hiding this comment.
Handle potential KeyError exceptions when accessing dictionary keys
In blockchain_message_from_dune_df_row, accessing dictionary keys directly (e.g., x["hash"]) without verifying their existence can lead to KeyError exceptions if any keys are missing from the x dictionary. This could cause the application to crash unexpectedly when processing incomplete data from external sources.
Consider modifying the method to handle missing keys gracefully:
def blockchain_message_from_dune_df_row(
self, consumer_address: ChecksumAddress, x: dict[str, Any]
) -> BlockchainMessage:
return BlockchainMessage(
consumer_address=consumer_address,
- transaction_hash=x["hash"],
- value_wei=str(x["value"]),
- block=str(x["block_number"]),
- sender_address=x["from"],
- data_field=self.unzip_message_else_do_nothing(x["data"]),
+ transaction_hash=x.get("hash", ""),
+ value_wei=str(x.get("value", 0)),
+ block=str(x.get("block_number", 0)),
+ sender_address=x.get("from", ""),
+ data_field=self.unzip_message_else_do_nothing(x.get("data", "")),
)This ensures that missing keys default to safe values, preventing KeyError exceptions.
📝 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.
| def blockchain_message_from_dune_df_row( | |
| self, consumer_address: ChecksumAddress, x: dict[str, Any] | |
| ) -> BlockchainMessage: | |
| return BlockchainMessage( | |
| consumer_address=consumer_address, | |
| transaction_hash=x["hash"], | |
| value_wei=str(x["value"]), | |
| block=str(x["block_number"]), | |
| sender_address=x["from"], | |
| data_field=self.unzip_message_else_do_nothing(x["data"]), | |
| ) | |
| def blockchain_message_from_dune_df_row( | |
| self, consumer_address: ChecksumAddress, x: dict[str, Any] | |
| ) -> BlockchainMessage: | |
| return BlockchainMessage( | |
| consumer_address=consumer_address, | |
| transaction_hash=x.get("hash", ""), | |
| value_wei=str(x.get("value", 0)), | |
| block=str(x.get("block_number", 0)), | |
| sender_address=x.get("from", ""), | |
| data_field=self.unzip_message_else_do_nothing(x.get("data", "")), | |
| ) |
| transactions = self.fetch_unseen_transactions(consumer_address=consumer_address) | ||
| return len(transactions) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid fetching all transactions just to get the count
Calling fetch_unseen_transactions to get the count of unprocessed transactions fetches all transaction data, which is inefficient and could impact performance.
Refactor fetch_count_unprocessed_transactions to fetch only the count from the database:
def fetch_count_unprocessed_transactions(
self, consumer_address: ChecksumAddress
) -> int:
- transactions = self.fetch_unseen_transactions(consumer_address=consumer_address)
- return len(transactions)
+ keys = APIKeys()
+ latest_blockchain_message = (
+ self.blockchain_table_handler.fetch_latest_blockchain_message(
+ consumer_address
+ )
+ )
+ min_block_number = (
+ 0 if not latest_blockchain_message else latest_blockchain_message.block
+ )
+ query = f'SELECT COUNT(*) AS count FROM gnosis.transactions WHERE "to" = {Web3.to_checksum_address(consumer_address)} AND block_number >= {min_block_number} AND value >= {xdai_to_wei(MicrochainAgentKeys().RECEIVER_MINIMUM_AMOUNT)}'
+ df = spice.query(query, api_key=keys.dune_api_key.get_secret_value())
+ existing_hashes = self.blockchain_table_handler.fetch_all_transaction_hashes(
+ consumer_address=consumer_address
+ )
+ total_count = df.select("count")[0, 0]
+ unseen_count = total_count - len(existing_hashes)
+ return max(unseen_count, 0)This change reduces unnecessary data retrieval and improves performance.
Committable suggestion skipped: line range outside the PR's diff.
| transactions = self.fetch_unseen_transactions(consumer_address=consumer_address) | ||
| if not transactions: | ||
| return None | ||
|
|
||
| # We only want the oldest non-processed message. | ||
| oldest_non_processed_message = df.row(0, named=True) | ||
| blockchain_message = BlockchainMessage( | ||
| consumer_address=consumer_address, | ||
| transaction_hash=oldest_non_processed_message["hash"], | ||
| value_wei=str(oldest_non_processed_message["value"]), | ||
| block=str(oldest_non_processed_message["block_number"]), | ||
| sender_address=oldest_non_processed_message["from"], | ||
| data_field=self.unzip_message_else_do_nothing( | ||
| oldest_non_processed_message["data"] | ||
| ), | ||
| ) | ||
| blockchain_message = transactions[0] | ||
|
|
||
| # Store here to avoid having to refresh after session was closed. | ||
| item = blockchain_message.model_copy(deep=True) | ||
| # mark unseen transaction as processed in DB | ||
| self.blockchain_table_handler.save_multiple([blockchain_message]) | ||
| return item | ||
| return blockchain_message |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize fetching of a single unprocessed transaction
Fetching all unseen transactions and selecting the first one is inefficient, especially when only one transaction is needed.
Modify the method to fetch only the oldest unseen transaction:
def fetch_one_unprocessed_blockchain_message_and_store_as_processed(
self, consumer_address: ChecksumAddress
) -> BlockchainMessage | None:
"""
Method for fetching the oldest unprocessed transaction sent to the consumer address.
After being fetched, it is stored in the DB as processed.
"""
- transactions = self.fetch_unseen_transactions(consumer_address=consumer_address)
- if not transactions:
+ transactions = self.fetch_unseen_transactions(consumer_address=consumer_address, limit=1)
+ if not transactions:
return None
blockchain_message = transactions[0]
# Mark unseen transaction as processed in DB
self.blockchain_table_handler.save_multiple([blockchain_message])
return blockchain_messageAdjust fetch_unseen_transactions to accept a limit parameter and modify the query accordingly to fetch only the required transaction.
Committable suggestion skipped: line range outside the PR's diff.
| st.markdown( | ||
| f""" | ||
| **From:** {transaction.sender_address} | ||
| **Message:** {transaction.data_field} | ||
| **Value:** {wei_to_xdai(transaction.value_wei_parsed)} xDai | ||
| """ | ||
| ) |
There was a problem hiding this comment.
Ensure proper sanitization when displaying user-generated content
Displaying transaction.data_field, which may contain user-generated content, using st.markdown can pose security risks if the content includes malicious code.
Verify that st.markdown is safe from injection attacks in this context. If necessary, escape or sanitize transaction.data_field before rendering:
import html
st.markdown(
f"""
**From:** {transaction.sender_address}
**Message:** {html.escape(transaction.data_field)}
**Value:** {wei_to_xdai(transaction.value_wei_parsed)} xDai
"""
)This ensures that any HTML or special characters in data_field are properly escaped, mitigating potential security vulnerabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
prediction_market_agent/db/blockchain_transaction_fetcher.py (1)
Line range hint
44-72: Protect against SQL injection and optimize query performanceThe SQL query construction using string formatting is vulnerable to SQL injection. Additionally, fetching all columns (*) may impact performance unnecessarily.
- Use parameterized queries to prevent SQL injection:
-query = f'select * from gnosis.transactions where "to" = {Web3.to_checksum_address(consumer_address)} AND block_number >= {min_block_number} and value >= {xdai_to_wei(MicrochainAgentKeys().RECEIVER_MINIMUM_AMOUNT)} order by block_time asc' +query = ''' + SELECT hash, value, block_number, "from", data, block_time + FROM gnosis.transactions + WHERE "to" = :consumer_address + AND block_number >= :min_block_number + AND value >= :min_value + ORDER BY block_time ASC +''' +params = { + "consumer_address": Web3.to_checksum_address(consumer_address), + "min_block_number": min_block_number, + "min_value": xdai_to_wei(MicrochainAgentKeys().RECEIVER_MINIMUM_AMOUNT) +} +df = spice.query(query, params, api_key=keys.dune_api_key.get_secret_value())
- Consider adding a LIMIT clause to prevent memory issues with large result sets:
# Add to query string: LIMIT 1000 # Adjust based on your needs
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
prediction_market_agent/db/blockchain_transaction_fetcher.py(4 hunks)
🔇 Additional comments (3)
prediction_market_agent/db/blockchain_transaction_fetcher.py (3)
32-42: 🛠️ Refactor suggestion
Add robust error handling for dictionary access
The direct dictionary key access without validation is still a concern, as previously noted. Additionally, the method should validate the data types of the values retrieved.
def blockchain_message_from_dune_df_row(
self, consumer_address: ChecksumAddress, x: dict[str, Any]
) -> BlockchainMessage:
+ try:
+ # Validate required fields exist and have correct types
+ required_fields = {"hash", "value", "block_number", "from", "data"}
+ if missing := required_fields - x.keys():
+ raise ValueError(f"Missing required fields: {missing}")
+
return BlockchainMessage(
consumer_address=consumer_address,
- transaction_hash=x["hash"],
- value_wei=str(x["value"]),
- block=str(x["block_number"]),
- sender_address=x["from"],
- data_field=self.unzip_message_else_do_nothing(x["data"]),
+ transaction_hash=x.get("hash", ""),
+ value_wei=str(x.get("value", 0)),
+ block=str(x.get("block_number", 0)),
+ sender_address=x.get("from", ""),
+ data_field=self.unzip_message_else_do_nothing(x.get("data", "")),
)
+ except Exception as e:
+ raise ValueError(f"Failed to create BlockchainMessage from row: {e}") from e77-78: 🛠️ Refactor suggestion
Optimize transaction count retrieval
Fetching all transactions just to get the count is inefficient, as previously noted. A COUNT query would be more performant.
87-96: 🛠️ Refactor suggestion
Optimize single transaction fetch and add error handling
Fetching all transactions when only one is needed is inefficient, as previously noted. Additionally, the save operation should include error handling.
def fetch_one_unprocessed_blockchain_message_and_store_as_processed(
self, consumer_address: ChecksumAddress
) -> BlockchainMessage | None:
"""
Method for fetching oldest unprocessed transaction sent to the consumer address.
After being fetched, it is stored in the DB as processed.
"""
- transactions = self.fetch_unseen_transactions(consumer_address=consumer_address)
+ transactions = self.fetch_unseen_transactions(consumer_address=consumer_address, limit=1)
if not transactions:
return None
blockchain_message = transactions[0]
- # mark unseen transaction as processed in DB
- self.blockchain_table_handler.save_multiple([blockchain_message])
+ try:
+ # mark unseen transaction as processed in DB
+ self.blockchain_table_handler.save_multiple([blockchain_message])
+ except Exception as e:
+ # Log the error but don't raise to prevent reprocessing the same transaction
+ logger.error(f"Failed to mark transaction as processed: {e}")
+
return blockchain_message
