Conversation
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Go bridge bindings and Python TNClient bridge methods, updates go.mod, adds ERC20 and TrufBridge ABIs, and introduces bridging example scripts and docs (deposit and withdraw/claim flows). Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DepositScript as "deposit_tt.py"
participant Web3 as "Web3 / Ethereum RPC"
participant ERC20 as "ERC20 Contract"
participant Bridge as "TrufBridge Contract"
User->>DepositScript: run deposit_tt.py
DepositScript->>Web3: connect to RPC
DepositScript->>ERC20: balanceOf(bot)
Web3-->>DepositScript: balance
DepositScript->>ERC20: approve(bridge, amount)
ERC20-->>Web3: approval tx
Web3-->>DepositScript: approval receipt
DepositScript->>Bridge: deposit(amount)
Bridge-->>Web3: deposit tx
Web3-->>DepositScript: deposit receipt
DepositScript-->>User: deposit confirmed
sequenceDiagram
actor User
participant WithdrawScript as "withdraw_and_claim.py"
participant Kwil as "TNClient / Kwil"
participant Web3 as "Web3 / Ethereum RPC"
participant Bridge as "TrufBridge Contract"
User->>WithdrawScript: run withdraw_and_claim.py
WithdrawScript->>Kwil: get_wallet_balance / get_history
Kwil-->>WithdrawScript: balance / history
WithdrawScript->>Kwil: withdraw(burn) -> txHash
Kwil-->>WithdrawScript: burn txHash
loop poll until proof ready
WithdrawScript->>Kwil: get_withdrawal_proof / get_history
Kwil-->>WithdrawScript: proof / status
WithdrawScript->>WithdrawScript: wait & retry
end
WithdrawScript->>WithdrawScript: parse_withdrawal_proof(proof)
WithdrawScript->>Bridge: submit claim tx(with proof)
Bridge-->>Web3: send claim tx
Web3-->>WithdrawScript: claim receipt
WithdrawScript-->>User: claim completed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
examples/bridging/deposit_tt.py (1)
21-23: Prefer env var for private key in examples.Using an environment variable avoids accidental commits and is closer to production usage.
♻️ Suggested update
- private_key = "<your_private_key_here>" # Replace with your bot's private key + private_key = os.getenv("TRUF_PRIVATE_KEY", "").strip() + if not private_key or private_key.startswith("<"): + print("[!] Set TRUF_PRIVATE_KEY in your environment.") + sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/bridging/deposit_tt.py` around lines 21 - 23, Replace the hard-coded private_key string with reading from an environment variable to avoid accidental commits; update the code around the private_key assignment in deposit_tt.py to fetch os.environ["BOT_PRIVATE_KEY"] (or os.environ.get("BOT_PRIVATE_KEY") with a clear fallback/error) and add a short runtime check that raises/prints an instructive error if the env var is missing so the example fails fast and documents the required BOT_PRIVATE_KEY environment variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/api-reference.md`:
- Around line 1365-1368: The markdown heading for
TNClient.parse_attestation_payload is currently using h4 (####) after an h2,
which triggers MD001; change the heading level to h3 (###) so it reads "###
`TNClient.parse_attestation_payload(payload: bytes) -> Dict[str, Any]`" to
maintain a single-level increment and fix the lint error; update the heading in
the docs around the Attestation Helpers section where
TNClient.parse_attestation_payload is declared.
In `@examples/bridging/withdraw_and_claim.py`:
- Around line 140-155: The prints inside withdraw_and_claim.py use f-strings
with no placeholders (e.g., print(f"\n[+] Withdrawal ready! Fetching proof...")
and print(f"\n[!] This withdrawal is already claimed.")), which triggers Ruff
F541; change those to normal strings (remove the "f" prefix) and also fix the
similar occurrence later around the second print at line ~168; keep all logic
(target_tx, status, proofs = tn_client.get_withdrawal_proof(...), proof =
proofs[0]) unchanged.
- Around line 100-118: The current try/except blocks around
tn_client.get_wallet_balance and tn_client.withdraw (and similar blocks at the
other locations mentioned) use blanket Exception and either exit or silently
swallow errors; update these to catch and handle specific exceptions (e.g.,
network/timeouts or client-specific errors) or at minimum log the full exception
via logger.error/print and re-raise or continue as appropriate; for the polling
loop referenced in the comment, do not silently pass—log the exception with
context (include function names like tn_client.get_wallet_balance and
tn_client.withdraw and any tx identifiers) and either retry with backoff or
break/raise so failures are visible instead of suppressed.
- Around line 29-45: The loop over proof signatures (raw_signatures -> for
sig_b64 ...) assumes 65-byte signatures and slices sig_bytes into r/s/v which
will raise IndexError for malformed inputs; before slicing sig_bytes check its
length (e.g., ensure len(sig_bytes) >= 65) and handle failures by either
skipping the entry and logging/warning or raising a clear ValueError; update the
logic around sig_b64/sig_bytes and when appending to formatted_signatures to
only process valid-length signatures and emit an explanatory message for
malformed ones.
In `@src/trufnetwork_sdk_py/client.py`:
- Around line 2754-2809: There are two get_history methods defined in this class
(one that calls truf_sdk.GetHistory and a later duplicate), causing the first to
be shadowed; locate both get_history definitions, decide which implementation to
keep (prefer keeping the binding-based get_history that calls
truf_sdk.GetHistory or the later version if it has additional logic), remove the
other duplicate (or rename it if you need both versions, e.g.,
get_history_legacy/get_history_v2), and update any callers or tests to reference
the kept method name and its docstring (refer to the method symbol get_history
and the binding call truf_sdk.GetHistory to find the binding-based
implementation and the duplicate definition to remove/rename).
---
Nitpick comments:
In `@examples/bridging/deposit_tt.py`:
- Around line 21-23: Replace the hard-coded private_key string with reading from
an environment variable to avoid accidental commits; update the code around the
private_key assignment in deposit_tt.py to fetch os.environ["BOT_PRIVATE_KEY"]
(or os.environ.get("BOT_PRIVATE_KEY") with a clear fallback/error) and add a
short runtime check that raises/prints an instructive error if the env var is
missing so the example fails fast and documents the required BOT_PRIVATE_KEY
environment variable.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/api-reference.md (1)
911-932:⚠️ Potential issue | 🟡 Minor
get_historyparameter name mismatch: docs saywallet_addressbut code useswallet.The documented signature at Line 911 is
get_history(bridge_identifier, wallet_address, ...)but the actual implementation atclient.pyLine 2795 useswalletas the parameter name. Users following the docs with keyword arguments (wallet_address="0x...") will get aTypeError.Proposed fix (align docs to code)
-### `client.get_history(bridge_identifier: str, wallet_address: str, limit: int = 20, offset: int = 0) -> List[BridgeHistory]` +### `client.get_history(bridge_identifier: str, wallet: str, limit: int = 20, offset: int = 0) -> List[BridgeHistory]` Retrieves the transaction history for a wallet on a specific bridge. #### Parameters - `bridge_identifier: str` - The name of the bridge instance (e.g., "hoodi_tt2") -- `wallet_address: str` - The wallet address to query +- `wallet: str` - The wallet address to query🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api-reference.md` around lines 911 - 932, The docs for client.get_history use the parameter name wallet_address but the implementation expects wallet; update the documentation to match the code by changing the signature and all examples to use wallet (e.g., get_history(bridge_identifier: str, wallet: str, ...)) and ensure example calls use wallet="0x..." so keyword-argument usage matches the get_history implementation (referencing the get_history function and the wallet parameter in client.py).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/api-reference.md`:
- Around line 1374-1384: The docs for parse_attestation_payload are inconsistent
with the ParsedAttestationPayload model in client.py; update the Returns section
to match the model by renaming the `args: List[Any]` entry to `arguments:
list[Any]` (or `arguments: List[Any]` to match doc style) and change `result:
bytes` to `result: list[dict[str, Any]]`, ensuring the field names and types
exactly match ParsedAttestationPayload and referencing parse_attestation_payload
and ParsedAttestationPayload so future reviewers can locate the source of truth.
In `@src/trufnetwork_sdk_py/client.py`:
- Around line 2781-2793: The get_withdrawal_proof method currently calls
truf_sdk.GetWithdrawalProof and directly json.loads the returned string, which
will raise json.JSONDecodeError on an empty string; update get_withdrawal_proof
to check the returned json_str from truf_sdk.GetWithdrawalProof (using
self.client, bridge_identifier, wallet) and return an empty list ([]) when
json_str is falsy, otherwise return json.loads(json_str); apply the same
defensive pattern used in get_order_book (and similarly for get_history) so
callers always receive [] instead of raising on empty responses.
- Around line 2767-2779: The withdraw method is missing a wait parameter and
should mirror other write methods: add a parameter wait: bool = True, call
truf_sdk.Withdraw(...) and store its return value in a tx_hash variable, and if
wait is True call self.wait_for_tx(tx_hash) before returning tx_hash; reference
the withdraw method, the truf_sdk.Withdraw call, and the self.wait_for_tx method
when making these changes.
---
Outside diff comments:
In `@docs/api-reference.md`:
- Around line 911-932: The docs for client.get_history use the parameter name
wallet_address but the implementation expects wallet; update the documentation
to match the code by changing the signature and all examples to use wallet
(e.g., get_history(bridge_identifier: str, wallet: str, ...)) and ensure example
calls use wallet="0x..." so keyword-argument usage matches the get_history
implementation (referencing the get_history function and the wallet parameter in
client.py).
---
Duplicate comments:
In `@examples/bridging/withdraw_and_claim.py`:
- Around line 163-166: Un-suppress polling exceptions in withdraw_and_claim.py:
replace the silent "except Exception as e: pass" in the polling loop with active
logging by uncommenting the existing print line (print(f"\n[!] Polling error:
{e}")) or using your module logger (e.g., logger.error/exception) to emit the
error details; this change should be made where the polling loop's exception
handler appears so transient network/auth errors are visible for debugging.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/trufnetwork_sdk_py/client.py (1)
2787-2819:get_withdrawal_proofandget_historyshould usecastfor consistency.Both methods return
json.loads(json_str)without acast, while every comparable read method in this class (get_order_book,get_user_positions,get_market_depth,get_best_prices,get_distribution_summary,get_distribution_details,get_participant_reward_history) wraps itsjson.loadsresult withcast. This lets type-checkers (mypy/pyright) enforce the declared return types.♻️ Proposed fix
def get_withdrawal_proof(self, bridge_identifier: str, wallet: str) -> list[dict]: ... json_str = truf_sdk.GetWithdrawalProof(self.client, bridge_identifier, wallet) if not json_str: return [] - return json.loads(json_str) + return cast(list[dict], json.loads(json_str)) def get_history(self, bridge_identifier: str, wallet: str, limit: int = 20, offset: int = 0) -> list[BridgeHistory]: ... json_str = truf_sdk.GetHistory(self.client, bridge_identifier, wallet, limit, offset) if not json_str: return [] - return json.loads(json_str) + return cast(list[BridgeHistory], json.loads(json_str))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trufnetwork_sdk_py/client.py` around lines 2787 - 2819, Both get_withdrawal_proof and get_history return json.loads(...) without a typing cast; update get_withdrawal_proof to return cast(list[dict], json.loads(json_str)) and get_history to return cast(list[BridgeHistory], json.loads(json_str)) (and add/import cast from typing at the top if not already present) so the runtime JSON parsing matches the declared return types and satisfies static type-checkers; keep the empty-list early returns as-is.examples/bridging/withdraw_and_claim.py (1)
153-157:proofs[0]may return the wrong proof when multiple withdrawals are pending.
get_withdrawal_proofreturns proofs for all unclaimed withdrawals for the wallet — not just the one initiated on Line 121. Picking index 0 will silently select an older or unrelated withdrawal if the wallet has concurrent pending claims, potentially submitting a claim for the wrong amount.♻️ Match proof to the current burn TX or by amount
# Fetch the actual proof data proofs = tn_client.get_withdrawal_proof(bridge_id, my_address) if proofs and len(proofs) > 0: - # Find the specific proof matching our amount/time if needed - proof = proofs[0] + # Match the proof to our specific withdrawal by amount + matched = [p for p in proofs if p.get('amount') == amount_str] + proof = matched[0] if matched else proofs[0] break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/bridging/withdraw_and_claim.py` around lines 153 - 157, The code currently uses proofs[0] which can pick the wrong withdrawal when multiple unclaimed withdrawals exist; update the logic after calling tn_client.get_withdrawal_proof(bridge_id, my_address) to find the proof that matches the specific withdrawal you just initiated (e.g., match by burn transaction hash or expected amount and timestamp), e.g. filter the returned proofs for proof.burn_tx_hash == burn_tx_hash or proof.amount == expected_amount (and optionally proof.timestamp within a small window), assign that matched proof to the proof variable, and handle the no-match case by logging/raising an error instead of blindly using proofs[0]; refer to tn_client.get_withdrawal_proof and the local proof variable to locate and change the selection logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/api-reference.md`:
- Around line 1382-1383: Update the docs to correctly describe
ParsedAttestationPayload.result as decoded data: change the parenthetical from
"(ABI-encoded result)" to something like "(decoded rows)" or "(decoded result
rows)" so it matches the client.py docstring and the type List[Dict[str, Any]];
ensure the symbol ParsedAttestationPayload.result is mentioned and the wording
aligns with "List of decoded rows from the query result."
- Around line 1406-1427: The docs for client.withdraw are missing the wait
parameter; update the documented signature for client.withdraw to include wait:
bool = True, add a Parameters entry describing wait (bool, default True)
explaining that when False the call is fire-and-forget and when True the method
waits for confirmation, and update the example to show both a default (awaiting)
call and a fire-and-forget call with wait=False so callers see the behavior and
default value.
---
Duplicate comments:
In `@examples/bridging/withdraw_and_claim.py`:
- Around line 86-92: Replace each broad "except Exception" with narrow, specific
exception handlers matching the call site: for the wallet load (account =
w3.eth.account.from_key(private_key)) catch the key/format errors such as
ValueError and eth_keys.exceptions.ValidationError (or the specific exception
from eth-account), for address/contract lookups catch
web3.exceptions.InvalidAddress/ValueError, for network or RPC calls catch
requests.exceptions.RequestException or web3.exceptions.TimeExhausted as
appropriate, and for the polling loop catch KeyboardInterrupt to allow clean
exit and specific timeout exceptions; in all handlers log the error and
exit/handle, but re-raise any unexpected exceptions (or add a final bare
"except:" that re-raises) so only the expected error types are swallowed. Ensure
you update the except clauses around the symbols account =
w3.eth.account.from_key, any address/contract lookup calls, the polling loop,
and RPC/network calls so Ruff BLE001 is resolved.
---
Nitpick comments:
In `@examples/bridging/withdraw_and_claim.py`:
- Around line 153-157: The code currently uses proofs[0] which can pick the
wrong withdrawal when multiple unclaimed withdrawals exist; update the logic
after calling tn_client.get_withdrawal_proof(bridge_id, my_address) to find the
proof that matches the specific withdrawal you just initiated (e.g., match by
burn transaction hash or expected amount and timestamp), e.g. filter the
returned proofs for proof.burn_tx_hash == burn_tx_hash or proof.amount ==
expected_amount (and optionally proof.timestamp within a small window), assign
that matched proof to the proof variable, and handle the no-match case by
logging/raising an error instead of blindly using proofs[0]; refer to
tn_client.get_withdrawal_proof and the local proof variable to locate and change
the selection logic.
In `@src/trufnetwork_sdk_py/client.py`:
- Around line 2787-2819: Both get_withdrawal_proof and get_history return
json.loads(...) without a typing cast; update get_withdrawal_proof to return
cast(list[dict], json.loads(json_str)) and get_history to return
cast(list[BridgeHistory], json.loads(json_str)) (and add/import cast from typing
at the top if not already present) so the runtime JSON parsing matches the
declared return types and satisfies static type-checkers; keep the empty-list
early returns as-is.
MicBun
force-pushed
the
withdrawlProgamatic
branch
from
36f8895 to
5dc05a6
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/api-reference.md (1)
911-932:⚠️ Potential issue | 🟠 Major
get_historysignature documentswalletbut the active implementation expectswallet_address.The active
get_historyatclient.pyLine 3097 has the signatureget_history(self, bridge_identifier, wallet_address, ...). The documentation (and the dead first definition at Line 2803) uses the parameter namewallet. Any caller following the documented keyword-arg style —client.get_history(bridge_identifier="hoodi_tt", wallet="0x...")— will get aTypeErrorat runtime because the live method expectswallet_address.📝 Proposed fix — align with the active implementation
-### `client.get_history(bridge_identifier: str, wallet: str, limit: int = 20, offset: int = 0) -> List[BridgeHistory]` +### `client.get_history(bridge_identifier: str, wallet_address: str, limit: int = 20, offset: int = 0) -> List[BridgeHistory]` #### Parameters - `bridge_identifier: str` - The name of the bridge instance (e.g., "hoodi_tt2") -- `wallet: str` - The wallet address to query +- `wallet_address: str` - The wallet address to query ... ```python history = client.get_history( bridge_identifier="hoodi_tt2", - wallet="0x..." + wallet_address="0x..." )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api-reference.md` around lines 911 - 932, The docs for client.get_history use the parameter name wallet but the live implementation in client.get_history (client.py) expects wallet_address; update the API docs (signature line, parameter list, and example call) to use wallet_address instead of wallet so they match the active method, and also change the parameter description to read `wallet_address: str - The wallet address to query`; verify the top signature (`### client.get_history(...)`) and the example call both use wallet_address to avoid runtime TypeError when callers use keyword args.
🧹 Nitpick comments (1)
src/trufnetwork_sdk_py/client.py (1)
2754-2785:get_wallet_balanceandwithdrawskipbridge_identifiervalidation thatget_historyenforces.The
get_historyat Line 3097 guardsbridge_identifier not in VALID_BRIDGESand raisesValueError. These two methods pass the identifier straight to the Go layer, which gives a worse error message on invalid input.♻️ Proposed fix — add consistent validation
def get_wallet_balance(self, bridge_identifier: str, wallet_address: str) -> str: + if bridge_identifier not in VALID_BRIDGES: + raise ValueError(f"bridge_identifier must be one of: {', '.join(VALID_BRIDGES)}") return truf_sdk.GetWalletBalance(self.client, bridge_identifier, wallet_address) def withdraw(self, bridge_identifier: str, amount: str, recipient: str, wait: bool = True) -> str: + if bridge_identifier not in VALID_BRIDGES: + raise ValueError(f"bridge_identifier must be one of: {', '.join(VALID_BRIDGES)}") + if not amount or not amount.isdigit(): + raise ValueError("amount must be a non-empty numeric string") tx_hash = truf_sdk.Withdraw(self.client, bridge_identifier, amount, recipient)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trufnetwork_sdk_py/client.py` around lines 2754 - 2785, get_wallet_balance and withdraw bypass the same bridge_identifier validation used by get_history (which checks bridge_identifier not in VALID_BRIDGES and raises ValueError); add the same validation to both functions by checking bridge_identifier against VALID_BRIDGES at the start of get_wallet_balance and withdraw (raise the same ValueError message), then proceed to call truf_sdk.GetWalletBalance and truf_sdk.Withdraw (preserving the existing wait_for_tx behavior in withdraw).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/bridging/withdraw_and_claim.py`:
- Around line 86-91: After creating the Web3 instance (w3 =
Web3(Web3.HTTPProvider(hoodi_rpc_url))) and injecting the
ExtraDataToPOAMiddleware, add a connection guard using w3.is_connected(); if it
returns False, log/print a clear error (including hoodi_rpc_url) and exit
cleanly before proceeding to Account.from_key(private_key) and any contract
calls—this prevents later cryptic AttributeError/ValueError from methods invoked
on an unconnected w3.
- Around line 149-170: The amount comparison is brittle: tx['amount'] ==
amount_str and p['amount'] == amount_str can silently fail and get_history is
limited to 5 results; update the logic in the withdrawal polling/claim block to
normalize and compare numeric values (e.g., parse both tx['amount'] and
amount_str to an integer or Decimal before comparing) and eliminate string
formatting issues (case/whitespace), and increase the history fetch window or
implement paging when calling tn_client.get_history (and similarly ensure
tn_client.get_withdrawal_proof results are iterated across pages) so the correct
withdrawal/proof is reliably found (references: tn_client.get_history,
amount_str, tx['amount'], proofs, tn_client.get_withdrawal_proof).
- Line 74: The code uses a hardcoded private key as a fallback for the variable
private_key which is a security risk; remove the default value in the
os.environ.get call for BOT_PRIVATE_KEY and instead fail fast when the
environment variable is missing (e.g., check that private_key is truthy after
retrieval in the same scope where private_key is defined, and raise an exception
or exit with a clear error message referencing BOT_PRIVATE_KEY so the script
cannot proceed without an explicitly provided secret).
---
Outside diff comments:
In `@docs/api-reference.md`:
- Around line 911-932: The docs for client.get_history use the parameter name
wallet but the live implementation in client.get_history (client.py) expects
wallet_address; update the API docs (signature line, parameter list, and example
call) to use wallet_address instead of wallet so they match the active method,
and also change the parameter description to read `wallet_address: str - The
wallet address to query`; verify the top signature (`###
client.get_history(...)`) and the example call both use wallet_address to avoid
runtime TypeError when callers use keyword args.
---
Duplicate comments:
In `@docs/api-reference.md`:
- Around line 1406-1427: The documented signature for client.withdraw is missing
the optional wait parameter; update the docs for the client.withdraw function
(the heading and parameter list for client.withdraw) to include the parameter
"wait: bool = True" with a short description (e.g., controls whether the call
waits for the burn tx to be mined; set wait=False for fire-and-forget) and, if
needed, clarify how the return value behaves when wait is False; ensure the
example and parameter list reflect the new optional argument.
In `@examples/bridging/withdraw_and_claim.py`:
- Around line 118-135: The RequestException except blocks are dead because
TNClient calls Go bindings, so remove the requests import and the specific
except requests.RequestException handlers around get_wallet_balance,
tn_client.withdraw and the polling loop; consolidate each into a single except
Exception as e (or the more specific Go-binding exception type if available)
that logs the error and exits, ensuring you only catch exceptions actually
thrown by the Go bindings and avoid misleading layered catches.
- Around line 92-97: The broad except Exception handlers that print messages
like "print(f\"[!] Failed to initialize clients: {e}\")" and the similar
polling-loop catches should be replaced with narrower handling: in
client-initialization blocks keep catching configuration errors (ValueError,
KeyError) but for other failures catch specific SDK/IO/network errors (e.g.,
RuntimeError, requests.exceptions.RequestException or your SDK's specific error
class) and call sys.exit(1) after logging the full error/traceback; in the
polling loop only catch expected transient errors (network/timeouts, API
rate-limit/errors) and log+retry, but do not swallow SystemExit,
KeyboardInterrupt, AssertionError or other critical exceptions — re-raise those
immediately. Update the relevant except blocks that currently match the literal
strings shown in the diff so initialization errors terminate and polling errors
are logged and retried, and include full error details (traceback) in logs.
In `@src/trufnetwork_sdk_py/client.py`:
- Around line 2803-2819: The file contains two get_history definitions (one
calling truf_sdk.GetHistory and another using call_procedure), causing the first
to be dead code and a parameter-name mismatch (wallet vs wallet_address); remove
the earlier binding-based get_history (the def get_history that calls
truf_sdk.GetHistory) so the call_procedure implementation remains the single
source of truth, or alternatively delete the later call_procedure version and
promote the earlier one but rename its parameter wallet → wallet_address and
update any callers/docs to match; prefer removing the first definition to keep
the validated call_procedure get_history as the canonical implementation.
---
Nitpick comments:
In `@src/trufnetwork_sdk_py/client.py`:
- Around line 2754-2785: get_wallet_balance and withdraw bypass the same
bridge_identifier validation used by get_history (which checks bridge_identifier
not in VALID_BRIDGES and raises ValueError); add the same validation to both
functions by checking bridge_identifier against VALID_BRIDGES at the start of
get_wallet_balance and withdraw (raise the same ValueError message), then
proceed to call truf_sdk.GetWalletBalance and truf_sdk.Withdraw (preserving the
existing wait_for_tx behavior in withdraw).
5dc05a6 to
cbcf4da
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/trufnetwork_sdk_py/client.py (1)
2754-2765:get_wallet_balance(andget_withdrawal_proof,withdraw) lackbridge_identifiervalidation, unlike the effectiveget_history.The second
get_historyat Line 3118 validatesbridge_identifier not in VALID_BRIDGESand raises a clearValueError. The three new bridge methods silently pass invalid identifiers to the Go binding, producing cryptic errors. Consistent defensive validation across all bridge methods would improve debuggability.♻️ Proposed refactor (apply to all three methods)
def get_wallet_balance(self, bridge_identifier: str, wallet_address: str) -> str: + if bridge_identifier not in VALID_BRIDGES: + raise ValueError(f"bridge_identifier must be one of: {', '.join(VALID_BRIDGES)}") + if not wallet_address or wallet_address.strip() == "": + raise ValueError("wallet_address is required and cannot be empty") return truf_sdk.GetWalletBalance(self.client, bridge_identifier, wallet_address)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/trufnetwork_sdk_py/client.py` around lines 2754 - 2765, Add the same defensive validation used in get_history to the bridge methods get_wallet_balance, get_withdrawal_proof, and withdraw: check if bridge_identifier not in VALID_BRIDGES and raise a ValueError with a clear message before calling the truf_sdk bindings (e.g., in get_wallet_balance currently calling truf_sdk.GetWalletBalance(self.client, bridge_identifier, wallet_address)); this ensures invalid identifiers are rejected early and avoids cryptic errors from the Go binding.examples/bridging/withdraw_and_claim.py (3)
219-219: Add an explicit timeout towait_for_transaction_receipt.The default timeout (120 s) will silently block if the transaction is dropped. An explicit timeout with an error message is more user-friendly.
♻️ Proposed refactor
- receipt = w3.eth.wait_for_transaction_receipt(tx_hash) + receipt = w3.eth.wait_for_transaction_receipt(tx_hash, timeout=300)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/bridging/withdraw_and_claim.py` at line 219, The call to w3.eth.wait_for_transaction_receipt(tx_hash) can hang if the tx is dropped; update the code that sets receipt to call wait_for_transaction_receipt with an explicit timeout (e.g. timeout=300) and wrap it in a try/except that catches the Web3 TimeExhausted exception (or a generic TimeoutError) to raise or log a clear error including tx_hash; update references around receipt and tx_hash so the error path handles the missing receipt gracefully.
207-211: Prefer EIP-1559 transaction parameters over legacygasPrice.Using
gasPricecreates a type-0 legacy transaction. Hoodi testnet supports EIP-1559 (type-2); considermaxFeePerGas/maxPriorityFeePerGasfor better gas cost estimation.♻️ Proposed refactor
).build_transaction({ 'from': my_address, 'nonce': w3.eth.get_transaction_count(my_address), - 'gasPrice': w3.eth.gas_price, + 'maxFeePerGas': w3.eth.max_priority_fee + 2 * w3.eth.get_block('latest')['baseFeePerGas'], + 'maxPriorityFeePerGas': w3.eth.max_priority_fee, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/bridging/withdraw_and_claim.py` around lines 207 - 211, The transaction dict in withdraw_and_claim.py passed to build_transaction is using the legacy 'gasPrice' which creates a type-0 tx; change it to EIP-1559 fields by replacing 'gasPrice' with 'maxFeePerGas' and 'maxPriorityFeePerGas' (compute maxFeePerGas as baseFeePerGas + maxPriorityFeePerGas or derive values from w3.eth.get_block('latest').baseFeePerGas and w3.eth.max_priority_fee) inside the same build_transaction call so the transaction becomes type-2; update the transaction dict used when calling build_transaction({ ... }) in the withdraw/claim flow accordingly.
95-97: Ruff BLE001 still reported at all fiveexcept Exceptionsites.While the previous silent-pass issue was addressed, broad
Exceptioncatches remain flagged. Narrowing to specific exception types (e.g.,RuntimeError,ValueError) would satisfy the linter and make error semantics explicit.Also applies to: 121-123, 133-135, 182-183, 226-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/bridging/withdraw_and_claim.py` around lines 95 - 97, Replace the broad "except Exception as e" handlers in the try blocks (the client initialization block around line ~95 and the other blocks at ~121-123, ~133-135, ~182-183, ~226-227) with narrow, specific exceptions that reflect the operations being performed (e.g., use ValueError or TypeError for argument/parse errors, ConnectionError or TimeoutError for network/client connections, OSError for file/IO, RuntimeError for general runtime failures), update the corresponding print/log calls to include the specific exception variable, and for truly unexpected errors either let them propagate or re-raise after logging (do not silently catch Exception). Ensure each except clause mentions the concrete exception types rather than Exception so Ruff BLE001 is satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/bridging/withdraw_and_claim.py`:
- Around line 118-120: Remove the dead requests-specific exception handlers and
the unused import: delete the `import requests` and the `except
requests.RequestException as e:` blocks that surround calls to
`truf_sdk.GetWalletBalance`, `truf_sdk.Withdraw`, and `truf_sdk.GetHistory`
(they are unreachable because these CGo bindings raise generic Exceptions),
leaving the existing `except Exception` handlers to catch SDK errors; ensure no
other code depends on `requests` before removing the import.
- Line 214: The send_raw_transaction call uses signed_tx.raw_transaction which
isn't present in eth-account <0.13.x; update the attribute access for
compatibility by retrieving the raw bytes from SignedTransaction using a
fallback (e.g., attempt 'raw_transaction' then 'rawTransaction') before passing
to w3.eth.send_raw_transaction, ensuring you still pass the raw bytes expected
by send_raw_transaction; update the usage around the signed_tx variable in
withdraw_and_claim.py so it uses a compatibility getter rather than direct
signed_tx.raw_transaction.
In `@src/trufnetwork_sdk_py/client.py`:
- Around line 2803-2819: There are two duplicate get_history definitions causing
the earlier one (with parameter wallet) to be shadowed and producing an API
inconsistency; remove the first get_history implementation (the one using
parameters bridge_identifier, wallet, limit, offset) and keep the later
definition that uses wallet_address, then update any callers to use the unified
signature wallet_address if necessary so all code consistently calls
Client.get_history(bridge_identifier, wallet_address, limit, offset).
---
Duplicate comments:
In `@docs/api-reference.md`:
- Around line 1406-1427: The docs for client.withdraw are missing the optional
wait parameter; update the API reference for the function signature and
parameters to include wait: bool = True and describe its behavior (True waits
for confirmation, False returns immediately for fire-and-forget), ensuring the
signature line `client.withdraw(bridge_identifier: str, amount: str, recipient:
str, wait: bool = True) -> str` and the Parameters list mention wait; reference
the implemented method name client.withdraw to ensure consistency with the
implementation.
In `@examples/bridging/withdraw_and_claim.py`:
- Line 74: The code reads BOT_PRIVATE_KEY into private_key with a placeholder
default, which defers failure to Account.from_key() and yields an unclear
ValueError; after reading os.environ.get("BOT_PRIVATE_KEY",
"<your_private_key_here>") add an explicit fail-fast guard that checks
private_key for a real value (not the placeholder and not None/empty) and raises
a clear RuntimeError or prints a descriptive error and exits if missing, so
Account.from_key() is only called with a valid key.
- Around line 86-88: After creating the Web3 instance (w3 =
Web3(Web3.HTTPProvider(hoodi_rpc_url))) and injecting ExtraDataToPOAMiddleware,
add an explicit connectivity check using w3.is_connected() and exit or raise a
clear error if it returns False; update the block around w3 initialization to
validate the connection early (use w3.is_connected()) and log or raise a
descriptive message mentioning hoodi_rpc_url so downstream code (e.g., where
AttributeError/ValueError occurs later) fails fast and clearly.
- Around line 149-170: The amount comparisons in the history loop and proof loop
are brittle and limited by a small fixed history window: change the logic in the
block around tn_client.get_history(bridge_id, my_address, 5, 0) and
tn_client.get_withdrawal_proof(bridge_id, my_address) so that amounts are
normalized before comparison (e.g., parse both tx['amount'] and amount_str into
a canonical numeric type such as int or Decimal and compare numerically) and
increase/iterate the history retrieval (replace the fixed limit=5 with a larger
limit or a paginated loop calling tn_client.get_history until the matching
withdrawal is found or exhausted) so the matching by amount and recipient in the
loops reliably finds the target_tx and proof.
---
Nitpick comments:
In `@examples/bridging/withdraw_and_claim.py`:
- Line 219: The call to w3.eth.wait_for_transaction_receipt(tx_hash) can hang if
the tx is dropped; update the code that sets receipt to call
wait_for_transaction_receipt with an explicit timeout (e.g. timeout=300) and
wrap it in a try/except that catches the Web3 TimeExhausted exception (or a
generic TimeoutError) to raise or log a clear error including tx_hash; update
references around receipt and tx_hash so the error path handles the missing
receipt gracefully.
- Around line 207-211: The transaction dict in withdraw_and_claim.py passed to
build_transaction is using the legacy 'gasPrice' which creates a type-0 tx;
change it to EIP-1559 fields by replacing 'gasPrice' with 'maxFeePerGas' and
'maxPriorityFeePerGas' (compute maxFeePerGas as baseFeePerGas +
maxPriorityFeePerGas or derive values from
w3.eth.get_block('latest').baseFeePerGas and w3.eth.max_priority_fee) inside the
same build_transaction call so the transaction becomes type-2; update the
transaction dict used when calling build_transaction({ ... }) in the
withdraw/claim flow accordingly.
- Around line 95-97: Replace the broad "except Exception as e" handlers in the
try blocks (the client initialization block around line ~95 and the other blocks
at ~121-123, ~133-135, ~182-183, ~226-227) with narrow, specific exceptions that
reflect the operations being performed (e.g., use ValueError or TypeError for
argument/parse errors, ConnectionError or TimeoutError for network/client
connections, OSError for file/IO, RuntimeError for general runtime failures),
update the corresponding print/log calls to include the specific exception
variable, and for truly unexpected errors either let them propagate or re-raise
after logging (do not silently catch Exception). Ensure each except clause
mentions the concrete exception types rather than Exception so Ruff BLE001 is
satisfied.
In `@src/trufnetwork_sdk_py/client.py`:
- Around line 2754-2765: Add the same defensive validation used in get_history
to the bridge methods get_wallet_balance, get_withdrawal_proof, and withdraw:
check if bridge_identifier not in VALID_BRIDGES and raise a ValueError with a
clear message before calling the truf_sdk bindings (e.g., in get_wallet_balance
currently calling truf_sdk.GetWalletBalance(self.client, bridge_identifier,
wallet_address)); this ensures invalid identifiers are rejected early and avoids
cryptic errors from the Go binding.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves: https://github.com/truflation/website/issues/3362
Summary by CodeRabbit
New Features
Examples
Documentation
Chores