feat: PoC redeem frontrun protection#335
Conversation
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
Reviewer's GuideImplements frontrun protection for redeem UTXO proposals by changing the Sui client to return structured success metadata, updating the redeem flow to react differently to successful vs rejected on-chain proposals, and doing minor documentation/table formatting cleanup. Sequence diagram for redeem UTXO frontrun protection flowsequenceDiagram
participant RedeemService
participant Storage
participant SuiClient
participant SuiContract
participant Logger
RedeemService->>Storage: getAvailableUtxos(setupId)
Storage-->>RedeemService: availableUtxos
RedeemService->>RedeemService: selectUtxos(availableUtxos, amount)
alt no_selected_utxos
RedeemService->>Logger: warn Insufficient UTXOs
RedeemService-->>RedeemService: return
else selected_utxos_found
RedeemService->>SuiClient: getSuiClient(suiNetwork)
RedeemService->>SuiClient: proposeRedeemUtxos(proposalArgs)
SuiClient->>SuiContract: submit transaction
SuiContract-->>SuiClient: transaction result
SuiClient-->>RedeemService: { success, digest }
alt on_chain_success
RedeemService->>Logger: info Proposed UTXOs for redeem request
RedeemService->>Storage: markRedeemProposed(redeemId, utxoIds, utxoLockTimeMs)
else contract_abort_better_solution
RedeemService->>Logger: info Proposal rejected, better solution exists
RedeemService->>Storage: markRedeemProposed(redeemId, [], 0)
end
end
opt network_or_unexpected_error
RedeemService->>Logger: error Failed to propose UTXOs
RedeemService-->>RedeemService: leave request pending for retry
end
Class diagram for updated SuiClient and RedeemService proposal logicclassDiagram
class ProposeRedeemCall {
}
class SuiClient {
+ikaClient() IkaClient
+proposeRedeemUtxos(args ProposeRedeemCall) Promise~ProposeRedeemResult~
+solveRedeemRequest(args SolveRedeemCall) Promise~string~
+finalizeRedeem(args FinalizeRedeemCall) Promise~string~
+requestIkaPresigns(count number) Promise~string[]~
}
class ProposeRedeemResult {
+success boolean
+digest string|null
}
class SuiClientImp {
+SuiClientImp(cfg SuiClientCfg)
+ikaClient() IkaClient
+proposeRedeemUtxos(args ProposeRedeemCall) Promise~ProposeRedeemResult~
+solveRedeemRequest(args SolveRedeemCall) Promise~string~
+finalizeRedeem(args FinalizeRedeemCall) Promise~string~
+requestIkaPresigns(count number) Promise~string[]~
-decodePresignId(encoded string) string
}
class RedeemService {
-utxoLockTimeMs number
+getSuiClient(suiNetwork string) SuiClient
+processRedeemRequest(req RedeemRequest) Promise~void~
}
class RedeemRequest {
+redeem_id string
+setup_id string
+amount number
+sui_network string
+nbtc_pkg string
+nbtc_contract string
}
class Storage {
+getAvailableUtxos(setupId string) Promise~Utxo[]~
+markRedeemProposed(redeemId string, utxoIds string[], lockTimeMs number) Promise~void~
}
class Utxo {
+nbtc_utxo_id string
}
SuiClientImp ..|> SuiClient
SuiClientImp --> ProposeRedeemResult
SuiClient --> ProposeRedeemCall
RedeemService --> SuiClient
RedeemService --> Storage
RedeemService --> RedeemRequest
Storage --> Utxo
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
sczembor
left a comment
There was a problem hiding this comment.
i think we should reconsider how to check if the solution we are about to propose is better than the existing one
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
sczembor
left a comment
There was a problem hiding this comment.
lets remove the dry-run and basically change the logic in that function to handle the netowrk fails and on-chain aborts and update the redeem status accrodingly. Either re-attempt or just assume its proposed because someone front run us
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Changing
SuiClient.proposeRedeemUtxosto return{ success, digest }instead of throwing on failure alters its error semantics; double-check all other call sites are updated to handle the new contract (including non-success statuses that are not frontrun-related). - In
proposeRedeemUtxos, you currently collapse all non-success transaction statuses intosuccess: false; consider returning or logging the on-chain error/status message so callers can distinguish frontrun-related aborts from other contract or gas failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing `SuiClient.proposeRedeemUtxos` to return `{ success, digest }` instead of throwing on failure alters its error semantics; double-check all other call sites are updated to handle the new contract (including non-success statuses that are not frontrun-related).
- In `proposeRedeemUtxos`, you currently collapse all non-success transaction statuses into `success: false`; consider returning or logging the on-chain error/status message so callers can distinguish frontrun-related aborts from other contract or gas failures.
## Individual Comments
### Comment 1
<location> `packages/sui-indexer/src/redeem-service.ts:349-358` </location>
<code_context>
- selectedUtxos.map((u) => u.nbtc_utxo_id),
- this.utxoLockTimeMs,
- );
+ if (result.success) {
+ logger.info({
+ msg: "Proposed UTXOs for redeem request",
+ redeemId: req.redeem_id,
+ txDigest: result.digest,
+ });
+ await this.storage.markRedeemProposed(
+ req.redeem_id,
+ selectedUtxos.map((u) => u.nbtc_utxo_id),
+ this.utxoLockTimeMs,
+ );
+ } else {
+ // Contract abort: someone already proposed a better solution.
+ // Mark as proposed without locking UTXOs so the redeem continue.
</code_context>
<issue_to_address>
**suggestion:** Non-success result is treated as "better proposal" but may also include other on-chain failure modes.
The `else` path currently treats every non-success as “better solution found” and advances the redeem without locking UTXOs. Other abort causes (invalid params, invariants, misconfig, etc.) would be misclassified and also advance the flow. Consider distinguishing specific abort reasons (e.g., via error codes/strings on `effects.status.error`) so you only skip locking for the "better solution" case and handle unexpected contract errors separately.
Suggested implementation:
```typescript
if (result.success) {
logger.info({
msg: "Proposed UTXOs for redeem request",
redeemId: req.redeem_id,
txDigest: result.digest,
});
await this.storage.markRedeemProposed(
req.redeem_id,
selectedUtxos.map((u) => u.nbtc_utxo_id),
this.utxoLockTimeMs,
);
} else {
// Distinguish "better proposal" abort from other on-chain failures.
// We only skip locking UTXOs when the contract explicitly reports
// that a better proposal already exists.
const errorMessage =
(result as any)?.effects?.status?.error ??
(result as any)?.error ??
"unknown on-chain error";
const isBetterProposalError =
typeof errorMessage === "string" &&
errorMessage.toLowerCase().includes("better proposal");
if (isBetterProposalError) {
// Contract abort: someone already proposed a better solution.
// Mark as proposed without locking UTXOs so the redeem can continue.
logger.info({
msg: "Proposal rejected on-chain, another worker proposed a better solution",
redeemId: req.redeem_id,
txDigest: result.digest,
error: errorMessage,
});
await this.storage.markRedeemProposed(req.redeem_id, [], 0);
} else {
// Unexpected contract error: do not advance redeem state.
// Surface the error so the catch block can handle it and allow retry.
logger.error({
msg: "Proposal rejected on-chain due to unexpected contract error",
redeemId: req.redeem_id,
txDigest: result.digest,
error: errorMessage,
});
throw new Error(
`Redeem proposal transaction failed with unexpected contract error: ${errorMessage}`,
);
}
}
} catch (e: unknown) {
// Network or unexpected error: leave as pending so next cron retry.
logError(
```
If the contract exposes a more precise error code (e.g. via `effects.status.error` being a structured string or enum like `"BetterRedeemProposalExists"`), update the `isBetterProposalError` predicate to match that exact value instead of using a `toLowerCase().includes("better proposal")` heuristic.
</issue_to_address>
### Comment 2
<location> `packages/sui-indexer/src/redeem-sui-client.ts:116-121` </location>
<code_context>
}
- async proposeRedeemUtxos(args: ProposeRedeemCall): Promise<string> {
+ async proposeRedeemUtxos(
+ args: ProposeRedeemCall,
+ ): Promise<{ success: boolean; digest: string | null }> {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The new return shape drops the detailed on-chain error, which could be useful for callers.
This change used to surface `result.effects?.status.error` via an exception, but now only returns `{ success, digest }`, losing the on-chain error detail. Consider adding `error: string | null` to the return type so callers can distinguish failure reasons and log richer context.
Suggested implementation:
```typescript
proposeRedeemUtxos(
args: ProposeRedeemCall,
): Promise<{ success: boolean; digest: string | null; error: string | null }>;
```
```typescript
async proposeRedeemUtxos(
args: ProposeRedeemCall,
): Promise<{ success: boolean; digest: string | null; error: string | null }> {
const tx = new Transaction();
tx.add(
```
To fully implement the suggestion and preserve the on-chain error context, you should:
1. Locate where the transaction is executed in `proposeRedeemUtxos` (e.g. `const result = await this.client.signAndExecuteTransactionBlock(...)` or similar).
2. When the transaction succeeds:
- Return `{ success: true, digest: result.digest, error: null }`.
3. When the transaction fails:
- Capture the on-chain error string (previously surfaced via `result.effects?.status.error` or via a thrown error) and return something like
`{ success: false, digest: result.digest ?? null, error: result.effects?.status.error ?? error.message ?? null }`.
4. Ensure any callers that previously relied on exceptions now read the `error` field when `success === false`, and update their typing accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
Signed-off-by: Rayane Charif <rayane.charif@gonative.cc>
Closes: #260
Summary by Sourcery
Implement on-chain-aware redeem UTXO proposal handling with basic frontrun protection and update Sui client response handling.
New Features:
Enhancements:
Documentation: