feat: Implement PaymentReceipt artifact and refine payment processing #53
Conversation
…f the remote agents, async_client and client_agent_imp
Summary of ChangesHello @jorellis, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on enhancing payment processing by introducing a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring of the agent interaction logic, moving towards a more robust and maintainable design by leveraging RemoteA2aAgent, AgentTool, and other ADK utilities. The introduction of the PaymentReceipt artifact is also a great feature, providing structured confirmation of transactions.
My review includes a few key points:
- Fixing a type error in payment nonce generation that was hidden by a
type: ignore. - Improving error handling during agent initialization to ensure it fails fast if the wallet is unavailable.
- Suggestions to replace hardcoded values (magic numbers and strings) with constants for better readability and maintainability.
| logger.error(f"Could not connect to mock wallet to get address: {e}") | ||
| # Handle the error appropriately, maybe by preventing initialization | ||
| return |
There was a problem hiding this comment.
If fetching the wallet address fails, the function logs an error and then returns, allowing the agent initialization to continue. This will cause self._wallet_address to be None, and any subsequent tool that relies on it (like pay_for_cart) will fail later with a less clear error. It's better to fail fast during initialization if a critical component like the wallet is unavailable.
| logger.error(f"Could not connect to mock wallet to get address: {e}") | |
| # Handle the error appropriately, maybe by preventing initialization | |
| return | |
| logger.error(f"Could not connect to mock wallet to get address: {e}") | |
| # Prevent agent from starting without a wallet address. | |
| raise RuntimeError("Failed to initialize wallet connection.") from e |
| valid_after=valid_after, | ||
| valid_before=valid_before, | ||
| nonce="0x" + os.urandom(32).hex(), | ||
| nonce="0x" + os.urandom(32).hex(), # type: ignore[arg-type] |
There was a problem hiding this comment.
The type: ignore[arg-type] here is hiding an underlying type mismatch. The get_transfer_with_auth_typed_data function expects nonce to be of type bytes, but it's being passed a hex string. This could lead to signing or transaction errors down the line. The correct approach is to pass the raw bytes directly from os.urandom(32).
| nonce="0x" + os.urandom(32).hex(), # type: ignore[arg-type] | |
| nonce=os.urandom(32), |
| "amount": {"currency": "USDC", "value": price_in_usd}, | ||
| } | ||
| ], | ||
| "total": { | ||
| "label": "Total", | ||
| "amount": {"currency": "USD", "value": price_in_usd}, | ||
| "amount": {"currency": "USDC", "value": price_in_usd}, |
There was a problem hiding this comment.
The currency is hardcoded as 'USDC'. To improve maintainability and avoid magic strings, it's better to define this as a constant at the top of the file (e.g., _CURRENCY = "USDC") and use it in all relevant places.
| ).get("id") | ||
| payment_id = f"payment_{uuid.uuid4()}" | ||
| price_in_usd = ( | ||
| f"{int(auth_dict.get("value")) / 1000000:.2f}" |
There was a problem hiding this comment.
The value 1000000 is used to convert the price from its smallest unit, assuming 6 decimal places for USDC. This is a 'magic number'. It would be more readable and maintainable to define this as a named constant (e.g., _USDC_DECIMALS = 1_000_000) or at least use underscores for readability.
| f"{int(auth_dict.get("value")) / 1000000:.2f}" | |
| f"{int(auth_dict.get("value")) / 1_000_000:.2f}" |
| agent=self.create_agent(), | ||
| rpc_url=url, | ||
| capabilities=capabilities, | ||
| agent_version="5.0.0", |
There was a problem hiding this comment.
The agent_version is hardcoded. It's good practice to define version strings as constants, typically at the module or class level (e.g., _AGENT_VERSION = "5.0.0"), to make them easier to find and update.
This PR introduces the following changes to the
adk_merchant_agent.py:process_paymentfunction now returns a structuredPaymentReceiptartifact upon successful transaction settlement, providing a comprehensive record of the payment.