fix: Change PaymentCurrencyAmount.value type from float to string#191
fix: Change PaymentCurrencyAmount.value type from float to string#191dinstein wants to merge 2 commits intogoogle-agentic-commerce:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, 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 addresses an inconsistency between the current implementation and the W3C Payment Request API specification by changing the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. Footnotes
|
082a3c0 to
1499de5
Compare
There was a problem hiding this comment.
Code Review
This pull request correctly aligns the PaymentCurrencyAmount.value type with the W3C specification by changing it from a numeric type to a string across the Python, Go, and Kotlin codebases. The changes are consistent and well-executed. My feedback focuses on improving the robustness of the implementation by adding error handling for string-to-numeric conversions and ensuring consistent formatting for currency values, which is crucial when dealing with monetary data.
| v, _ := strconv.ParseFloat(item.Amount.Value, 64) | ||
| newTotal += v |
There was a problem hiding this comment.
The error from strconv.ParseFloat is ignored. If an item's amount is not a valid number, it will be silently added as 0 to the total. This could lead to incorrect calculations. It's better to log this error for debugging purposes. For example:
v, err := strconv.ParseFloat(item.Amount.Value, 64)
if err != nil {
fmt.Fprintf(os.Stderr, "could not parse item value '%s', treating as 0: %v\n", item.Amount.Value, err)
}
newTotal += v| "asset": "0x833589fCD6eDb6E08f4c7C32D4f71b54bda02913", | ||
| "payTo": "0xMerchantWalletAddress", | ||
| "maxAmountRequired": str(int(item.amount.value * 1000000)) | ||
| "maxAmountRequired": str(int(float(item.amount.value) * 1000000)) |
| payment_request.details.total.amount.value = str(sum( | ||
| float(item.amount.value) for item in payment_request.details.display_items | ||
| )) |
There was a problem hiding this comment.
This block can be improved for robustness and formatting.
- Error Handling: The
float()conversion will raise aValueErrorif a value is not a valid number, which will crash the tool. - Currency Formatting:
str(sum(...))doesn't guarantee two decimal places for the currency value (e.g.,str(1.5)is'1.5').
Consider refactoring this to handle errors and format the output correctly. You may need to import logging.
total = 0
for item in payment_request.details.display_items:
try:
total += float(item.amount.value)
except (ValueError, TypeError):
logging.warning(f'Invalid value for item amount, skipping: {item.amount.value}')
payment_request.details.total.amount.value = f'{total:.2f}'1499de5 to
d0646f1
Compare
Per the W3C Payment Request API specification, the `value` field in PaymentCurrencyAmount is defined as a DOMString, not a number type. This change aligns the implementation across Python, Go, and Kotlin with the W3C standard for cross-language consistency. Updated type definitions: - Python: float -> str - Go: float64 -> string - Kotlin: Double -> String Updated all sample code to use string values and handle float conversions where arithmetic is needed.
d0646f1 to
21c348d
Compare
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Summary
The
valuefield inPaymentCurrencyAmountis currently defined as a numeric type (float/float64/Double), which is inconsistent with the W3C Payment Request API spec wherevalueis defined as aDOMString. This PR aligns the type definition with the W3C standard for cross-language consistency.Changes
PaymentCurrencyAmount.valuefrom numeric type to string across Python (float→str), Go (float64→string), and Kotlin (Double→String)"2.00"instead of2.00) and addfloat()/strconv.ParseFloat()conversions where arithmetic operations are neededFiles changed (8 files)
src/ap2/types/payment_request.pyfloat→strsamples/go/pkg/ap2/types/payment_request.gofloat64→stringsamples/android/.../data/ShoppingAgentTypes.ktDouble→Stringsamples/python/.../merchant_agent/tools.pyfloat()for sumsamples/python/.../sub_agents/catalog_agent.pyfloat()conversion for arithmeticsamples/go/.../merchant_agent/tools.gostrconvconversionssamples/go/.../merchant_agent/storage.gostrconv.FormatFloatfor Price→Valuesamples/android/.../agent/DpcHelper.kt.toString()callsTest plan