fix: validate payment timeout values - #130
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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds validation to ensure that max_timeout_seconds is a positive integer (and not a boolean) in create_payment_requirements, along with corresponding unit tests. The review feedback suggests improving the test coverage by asserting the specific exception messages and testing additional invalid types like None and stringified integers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for timeout in (0, -1, 1.5, True): | ||
| with pytest.raises(ValueError): | ||
| create_payment_requirements( | ||
| price="$1.00", | ||
| pay_to_address="0x123", | ||
| resource="/test", | ||
| max_timeout_seconds=timeout, | ||
| ) |
There was a problem hiding this comment.
The current test uses a generic pytest.raises(ValueError) without verifying the exception message. This can lead to false positives if a ValueError is raised from another part of the function (e.g., during price processing).
Additionally, we should expand the test coverage to include other common invalid types such as None and stringified integers (e.g., "60").
We can improve this by using pytest.raises(ValueError, match=...) and parameterizing or looping over the expected error messages.
| for timeout in (0, -1, 1.5, True): | |
| with pytest.raises(ValueError): | |
| create_payment_requirements( | |
| price="$1.00", | |
| pay_to_address="0x123", | |
| resource="/test", | |
| max_timeout_seconds=timeout, | |
| ) | |
| for timeout, expected_msg in [ | |
| (0, "max_timeout_seconds must be positive"), | |
| (-1, "max_timeout_seconds must be positive"), | |
| (1.5, "max_timeout_seconds must be an integer"), | |
| (True, "max_timeout_seconds must be an integer"), | |
| (None, "max_timeout_seconds must be an integer"), | |
| ("60", "max_timeout_seconds must be an integer"), | |
| ]: | |
| with pytest.raises(ValueError, match=expected_msg): | |
| create_payment_requirements( | |
| price="$1.00", | |
| pay_to_address="0x123", | |
| resource="/test", | |
| max_timeout_seconds=timeout, | |
| ) |
bf4b3d1 to
181ef11
Compare
samsamtrum
changed the title
1 participant
create_payment_requirementsaccepted invalid timeout values and passed them into the generated payment requirements.This adds a small validation step so the timeout must be a positive integer before the payment requirement is built. It also adds regression coverage for zero, negative, fractional, and boolean values.