Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves ensure_opg_approval reliability by waiting for the on-chain allowance change to become visible after an approval transaction is mined, preventing callers from immediately seeing stale allowance values.
Changes:
- Added configurable timeouts and polling interval for post-receipt allowance confirmation.
- Updated approval flow to poll
allowance()until the approved amount is observable (or timeout). - Added a unit test covering the “receipt mined but allowance not immediately updated” scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/opengradient/client/opg_token.py |
Adds approval receipt timeout constant and a post-receipt polling loop to confirm allowance updates. |
tests/opg_token_test.py |
Adds a test validating that ensure_opg_approval polls allowance after a successful receipt until the new value appears. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from dataclasses import dataclass | ||
| import time |
There was a problem hiding this comment.
Import ordering here is inconsistent with other modules in this repo (e.g., stdlib import ... lines come before from dataclasses import dataclass in src/opengradient/client/llm.py and src/opengradient/types.py). Reorder/group the stdlib imports (time, dataclasses, typing) to match the existing convention and avoid isort/ruff import-order noise.
| from dataclasses import dataclass | |
| import time | |
| import time | |
| from dataclasses import dataclass |
| deadline = time.time() + ALLOWANCE_CONFIRMATION_TIMEOUT | ||
| allowance_after = allowance_before | ||
| while allowance_after < amount_base: | ||
| allowance_after = token.functions.allowance(owner, spender).call() | ||
| if allowance_after >= amount_base: | ||
| break | ||
| if time.time() >= deadline: |
There was a problem hiding this comment.
Timeout logic should use a monotonic clock for reliability. Using time.time() can cause the polling loop to terminate too early or too late if system time changes (NTP adjustments, leap seconds, etc.). Consider switching to time.monotonic() for computing/checking deadline.
| deadline = time.time() + ALLOWANCE_CONFIRMATION_TIMEOUT | |
| allowance_after = allowance_before | |
| while allowance_after < amount_base: | |
| allowance_after = token.functions.allowance(owner, spender).call() | |
| if allowance_after >= amount_base: | |
| break | |
| if time.time() >= deadline: | |
| deadline = time.monotonic() + ALLOWANCE_CONFIRMATION_TIMEOUT | |
| allowance_after = allowance_before | |
| while allowance_after < amount_base: | |
| allowance_after = token.functions.allowance(owner, spender).call() | |
| if allowance_after >= amount_base: | |
| break | |
| if time.monotonic() >= deadline: |
| def test_waits_for_allowance_update_after_receipt(self, mock_wallet, mock_web3, monkeypatch): | ||
| """After a successful receipt, poll allowance until the updated value is visible.""" | ||
| monkeypatch.setattr("opengradient.client.opg_token.ALLOWANCE_POLL_INTERVAL", 0) | ||
| contract = _setup_allowance(mock_web3, 0) | ||
|
|
||
| approve_fn = MagicMock() | ||
| contract.functions.approve.return_value = approve_fn | ||
| approve_fn.estimate_gas.return_value = 50_000 | ||
| approve_fn.build_transaction.return_value = {"mock": "tx"} | ||
|
|
||
| mock_web3.eth.get_transaction_count.return_value = 0 | ||
| mock_web3.eth.gas_price = 1_000_000_000 | ||
| mock_web3.eth.chain_id = 84532 | ||
|
|
||
| signed = MagicMock() | ||
| signed.raw_transaction = b"\x00" | ||
| mock_wallet.sign_transaction.return_value = signed | ||
|
|
||
| tx_hash = MagicMock() | ||
| tx_hash.hex.return_value = "0xconfirmed" | ||
| mock_web3.eth.send_raw_transaction.return_value = tx_hash | ||
| mock_web3.eth.wait_for_transaction_receipt.return_value = SimpleNamespace(status=1, blockNumber=100) | ||
|
|
||
| amount_base = int(1.0 * 10**18) | ||
| contract.functions.allowance.return_value.call.side_effect = [0, 0, amount_base] |
There was a problem hiding this comment.
This test can hang for up to 120 seconds on a regression because it sets ALLOWANCE_POLL_INTERVAL = 0 but leaves ALLOWANCE_CONFIRMATION_TIMEOUT at its default. To keep failures fast and deterministic, also monkeypatch ALLOWANCE_CONFIRMATION_TIMEOUT to a small value (or patch the time source) so the loop exits quickly if the allowance never updates.
checks for approval limit during limit update