Conversation
Merges limit order support functions from PR MeteoraAg#273: isSupportLimitOrder determines pool type, getFeeMode dispatches fee collection mode based on CollectFeeMode enum. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…classes Adds ILimitOrder/ILimitOrderBinData interfaces, LimitOrderV1Wrapper, ParsedLimitOrder types, and wrapLimitOrder factory from PR MeteoraAg#273. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds LimitOrderBinData, PlaceLimitOrderParams, ParsedLimitOrderWithPubkey, LIMIT_ORDER_MIN_SIZE/BIN_DATA_SIZE constants, BinLiquidity limit order fields, feeOnInput to SwapQuote, PositionPermission and LimitOrderStatus enums from PR MeteoraAg#273. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ssions Adds export * from "./bin", includes LimitOrder in decodeAccount generic union, and adds encodePositionPermissions helper from PR MeteoraAg#273. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…aAg#273 Constants: adds BIN_ARRAY_DEFAULT_VERSION, LIMIT_ORDER_FEE_SHARE, ConcreteFunctionType, CollectFeeMode enums. AccountFilters: adds limitOrderFilter, limitOrderOwnerFilter, limitOrderLbPairFilter. Fee: adds swapExactInQuoteAtBin/swapExactOutQuoteAtBin alongside existing fee helpers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds helpers/bin.ts with limit-order-aware swapExactInQuoteAtBin, swapExactOutQuoteAtBin, getAmountIn/Out, and getLimitOrderLiquidity. Adds limitOrders/index.ts with decodeLimitOrderBinData. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/bot review |
|
Bot Agent progress:
Progress:
|
![metoeraag-bot[bot]](https://avatars.githubusercontent.com/u/126859799?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Kept all five comments: the PublicKey reference-comparison bug affecting LM/LO fallback selection, the limit-order bin serialization/deserialization gap including /dlmm/swap-quote, the zero-amount division risk in splitFee(), the missing await on getLatestBlockhash() in sendTransactionAndConfirm, and the missing zero-denominator guard in calculateLimitOrderFee().
Inline comments
See inline comments on the diff.
Claude (API) review
Rounds: 3 (min=1, max=10).
First look: Codex + Claude (API).
Agreement: yes (no new findings/questions).
Debate transcript
First look comparison
Codex (first look):
Summary:
Found three correctness regressions around the new limit-order support: Undetermined pools are misclassified because PublicKey values are compared by reference, empty-bin fee splitting can divide by zero, and the server still serializes/deserializes only the legacy bin fields needed before limit-order pools existed.
Inline comment highlights:
- ts-client/src/dlmm/helpers/lbPair.ts:55:
PublicKeyobjects should be compared with.equals(), not!=. This predicate is reference-based today, so decoded reward mints that equal the default key will still look "initialized" here. That makesFunctionType.Undeterminedpools - ts-client/src/server/index.ts:321: For limit-order pools this response still serializes only the legacy bin fields (
amountX/Y, fee-per-token, liquidity, price, rewards). The new quote logic reads additional LO fields from eachBin(openOrderAmount, `processedOrderRemai - ts-client/src/dlmm/helpers/fee.ts:124:
splitFee()now divides bytotalAmountInunconditionally. After the refactor,swapExactInQuoteAtBin()can reach this helper withtotalAmountIn == 0on zero-fill / empty-bin paths (the previous implementation short-circuited those cas
Claude (API) (first look):
Summary:
Release 0.12.0 sync: program ID updated, IDL upgraded with limit order support, new fee modes, and bin structure changes. Most changes are formatting (trailing commas) and feature additions. Found one concrete bug in the helper utility and several clippy lint failures in Rust code.
Inline comment highlights:
- ts-client/src/test/helper.ts:341: Bug:
connection.getLatestBlockhash()returns aPromise, but it is notawaited here. This meanslatestBlockhashwill be aPromiseobject, and the spread...latestBlockhashinto theTransactionconstructor will not provide the e - ts-client/src/dlmm/helpers/fee.ts:126: Potential division-by-zero: when
totalAmountInis zero (e.g., both MM and limit-order fills are zero),mmFeecomputation will divide by zero (totalAmountIn.sub(new BN(1)).div(totalAmountIn)). Consider adding a guard: ```ts if (totalAm - ts-client/src/dlmm/helpers/limitOrders/wrapper.ts:54: The
Undeterminedfunction type case inisSupportLimitOrderchecksrewardInfo.mint != PublicKey.default, but!=onPublicKeyobjects performs reference comparison, not value comparison. This will almost always returntrue(incorr - ts-client/src/dlmm/helpers/lbPair.ts:55: Reference equality issue:
rewardInfo.mint != PublicKey.defaultcompares object references, not public key bytes. This will almost always betrue, causingUndeterminedtype pools to always be treated as supporting limit orders. Use: `` - ts-client/src/server/index.ts:114: The default for
concreteFunctionTypewhen not provided isConcreteFunctionType.LimitOrder. For a server endpoint creating customizable permissionless LB pairs, defaulting toLimitOrderrather thanLiquidityMiningcould be surprising - ts-client/src/dlmm/helpers/fee.ts:124: The rounding term
totalAmountIn.sub(new BN(1))for the ceiling division ofmmFeecan underflow toU64_MAXiftotalAmountInis1(since BN may wrap). This specific case is unlikely to cause issues with BN's signed arithmetic, but i
Comparison notes:
Review synthesis
Top differences in emphasis
- Core blockers vs. broader cleanup: Both reviews agree on two key bugs:
PublicKeyshould use.equals()andsplitFee()needs a zero-amount guard. Review B adds extra issues (missingawait, clippy failures, surprising server default). - Server compatibility gap: Review A uniquely flags a likely release-blocking issue: the server still serializes only legacy bin fields, so limit-order quotes may be wrong/incomplete.
- Fee-path nuance: Review A focuses on divide-by-zero in empty/zero-fill paths; Review B also questions the rounding/underflow behavior around
totalAmountIn == 1.
Most important follow-up action
- Fix and regression-test the limit-order correctness path end-to-end, prioritizing:
PublicKey.equals()checks,splitFee()zero guards, and server serialization/deserialization of the new limit-order bin fields.
Round 1
Codex (primary):
Summary:
Found three correctness regressions around the new limit-order support: Undetermined pools are misclassified because PublicKey values are compared by reference, empty-bin fee splitting can divide by zero, and the server still serializes/deserializes only the legacy bin fields needed before limit-order pools existed.
Inline comment highlights:
- ts-client/src/dlmm/helpers/lbPair.ts:55:
PublicKeyobjects should be compared with.equals(), not!=. This predicate is reference-based today, so decoded reward mints that equal the default key will still look "initialized" here. That makesFunctionType.Undeterminedpools - ts-client/src/server/index.ts:321: For limit-order pools this response still serializes only the legacy bin fields (
amountX/Y, fee-per-token, liquidity, price, rewards). The new quote logic reads additional LO fields from eachBin(openOrderAmount, `processedOrderRemai - ts-client/src/dlmm/helpers/fee.ts:124:
splitFee()now divides bytotalAmountInunconditionally. After the refactor,swapExactInQuoteAtBin()can reach this helper withtotalAmountIn == 0on zero-fill / empty-bin paths (the previous implementation short-circuited those cas
Claude (API):
(no additional notes)
Codex (response):
- Open questions kept for author: 1/1
Round 2
Codex (primary):
Summary:
Retained the three correctness regressions from the prior review; the critic feedback did not provide any actionable false positives or new findings to change them.
Inline comment highlights:
- ts-client/src/dlmm/helpers/lbPair.ts:55:
PublicKeyobjects should be compared with.equals(), not!=. This predicate is reference-based today, so decoded reward mints that equal the default key will still look "initialized" here. That makesFunctionType.Undeterminedpools - ts-client/src/server/index.ts:321: For limit-order pools this response still serializes only the legacy bin fields (
amountX/Y, fee-per-token, liquidity, price, rewards). The new quote logic reads additional LO fields from eachBin(openOrderAmount, `processedOrderRemai - ts-client/src/dlmm/helpers/fee.ts:124:
splitFee()now divides bytotalAmountInunconditionally. After the refactor,swapExactInQuoteAtBin()can reach this helper withtotalAmountIn == 0on zero-fill / empty-bin paths (the previous implementation short-circuited those cas
Claude (API):
Critique
The primary review correctly identifies the three highest-impact issues:
PublicKeyreference equality (!=vs.equals()) inisSupportLimitOrder— agreed, this is a real bug.splitFeedivide-by-zero whentotalAmountInis zero — agreed, this needs a guard.- Server serialization gap for limit-order bin fields — agreed, this will produce wrong quotes.
However, the primary review missed two additional issues:
Missing await in test helper (high impact for test reliability)
The new sendTransactionAndConfirm helper in ts-client/src/test/helper.ts:341 calls connection.getLatestBlockhash() without await. This means the function is not async and the Transaction constructor receives a Promise object instead of blockhash data. Every test using this helper will fail. This was flagged in the independent first-look review but dropped from the primary review.
Division-by-zero in calculateLimitOrderFee
In wrapper.ts:428-434, the fee calculation divides fulfilledAmount by bin.fulfilledOrderAmountX (or Y). The guard at line 419 only checks if fulfilledAmount is zero, not whether the denominator is zero. Edge cases could trigger a division-by-zero.
The primary review's three findings are all valid and should be kept. No false positives identified.
Codex (response):
- New findings applied: 3/3
- Open questions kept for author: 1/1
Round 3
Codex (primary):
Summary:
Retained the original three correctness issues, clarified that the limit-order bin bug also affects /dlmm/swap-quote deserialization, and added the two missed findings from the critic: the missing await in sendTransactionAndConfirm and a denominator-zero guard in calculateLimitOrderFee.
Inline comment highlights:
- ts-client/src/dlmm/helpers/lbPair.ts:55:
PublicKeyvalues should be compared with.equals(), not!=. This check is reference-based today, so a decoded reward mint equal to the default key can still look "initialized" here. That makesFunctionType.Undeterminedpools take th - ts-client/src/server/index.ts:321: For limit-order pools this path still serializes only the legacy bin fields. The LO quote logic also needs the newer fields from each
Bin(openOrderAmount,processedOrderRemainingAmount,totalProcessingOrderAmount,orderAge, `limi - ts-client/src/dlmm/helpers/fee.ts:124:
splitFee()now divides bytotalAmountInunconditionally. After the refactor,swapExactInQuoteAtBin()can reach this helper withtotalAmountIn == 0on zero-fill / empty-bin paths (the previous implementation short-circuited those cas - ts-client/src/test/helper.ts:341:
connection.getLatestBlockhash()returns aPromise, but it is used synchronously here. Spreading...latestBlockhashinto theTransactionconstructor will spread thePromiseobject instead of{ blockhash, lastValidBlockHeight }, s - ts-client/src/dlmm/helpers/limitOrders/wrapper.ts:427:
calculateLimitOrderFee()guards onlyfulfilledAmount.isZero(), but the denominator in this division isbin.fulfilledOrderAmountX/Y. If that denominator is zero whilefulfilledAmount > 0(for example, in an inconsistent partially p
Claude (API):
Critique
The primary review now covers all five key issues identified across the independent first-look review and the prior critic rounds:
PublicKeyreference equality (!=vs.equals()) inisSupportLimitOrder— correctly flagged.splitFeedivide-by-zero whentotalAmountInis zero — correctly flagged.- Server serialization gap for limit-order bin fields — correctly flagged, including the deserialization side.
- Missing
awaitonconnection.getLatestBlockhash()insendTransactionAndConfirm— correctly flagged. - Denominator-zero guard in
calculateLimitOrderFee— correctly flagged.
The open question about isSupportLimitOrder semantics for the Undetermined case has been appropriately kept for the author.
All findings are well-substantiated with correct line references, impact descriptions, and suggested fixes. No false positives detected in the primary review. The clippy lint failures in the Rust commons crate are real but are environmental/CI issues rather than new logic bugs introduced in this PR, and were reasonably omitted from the primary review's scope.
No additional high-signal issues found that weren't already covered.
Codex (response):
Agreement reached. No further changes.
Synthesis
Review synthesis
Top differences in emphasis
- Core blockers vs. broader cleanup: Both reviews agree on two key bugs:
PublicKeyshould use.equals()andsplitFee()needs a zero-amount guard. Review B adds extra issues (missingawait, clippy failures, surprising server default). - Server compatibility gap: Review A uniquely flags a likely release-blocking issue: the server still serializes only legacy bin fields, so limit-order quotes may be wrong/incomplete.
- Fee-path nuance: Review A focuses on divide-by-zero in empty/zero-fill paths; Review B also questions the rounding/underflow behavior around
totalAmountIn == 1.
Most important follow-up action
- Fix and regression-test the limit-order correctness path end-to-end, prioritizing:
PublicKey.equals()checks,splitFee()zero guards, and server serialization/deserialization of the new limit-order bin fields.
Repo checks
Repo Checks
- rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu rustfmt: ok
info: downloading component 'rustfmt'
info: installing component 'rustfmt'
- rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu clippy: ok
info: downloading component 'clippy'
info: installing component 'clippy'
- cargo fetch --locked: ok
-pubsub-client v2.2.4
Downloaded walkdir v2.4.0
Downloaded predicates-core v1.0.8
Downloaded solana-zk-elgamal-proof-program v2.2.4
Downloaded solana-version v2.2.4
Downloaded solana-type-overrides v2.2.4
Downloaded solana-account-decoder v2.2.4
Downloaded solana-transaction-status-client-types v2.2.4
Downloaded proc-macro-crate v3.2.0
Downloaded writeable v0.5.5
Downloaded zerofrom v0.1.5
Downloaded quote v1.0.37
Downloaded syn v2.0.90
Downloaded anchor-attribute-access-control v0.31.0
Downloaded anchor-spl v0.31.0
Downloaded solana-thin-client v2.2.4
Downloaded deranged v0.3.9
Downloaded time v0.3.37
Downloaded unicode-width v0.2.0
Downloaded pin-project-internal v1.1.3
Downloaded solana-builtins v2.2.4
Downloaded crossbeam-deque v0.8.3
Downloaded filetime v0.2.23
Downloaded solana-compute-budget-program v2.2.4
Downloaded solana-stake-program v2.2.4
Downloaded http-body v0.4.5
Downloaded crc32fast v1.3.2
Downloaded icu_provider_macros v1.5.0
Downloaded ureq v2.12.1
Downloaded solana-rpc-client v2.2.4
Downloaded solana-rpc-client-api v2.2.4
Downloaded solana-tpu-client v2.2.4
Downloaded thread_local v1.1.7
Downloaded termtree v0.4.1
Downloaded predicates-tree v1.0.11
Downloaded solana-zk-token-proof-program v2.2.4
Downloaded getrandom v0.3.2
Downloaded errno v0.3.10
Downloaded solana-builtins-default-costs v2.2.4
Downloaded solana-program-runtime v2.2.4
Downloaded icu_locid_transform_data v1.5.0
Downloaded rustls-pki-types v1.11.0
Downloaded solana-address-lookup-table-program v2.2.4
Downloaded mio v1.0.3
Downloaded miniz_oxide v0.8.5
Downloaded rustls v0.23.28
Downloaded icu_provider v1.5.0
Downloaded icu_properties v1.5.1
Downloaded rustversion v1.0.18
Downloaded solana-compute-budget-instruction v2.2.4
Downloaded write16 v1.0.0
Downloaded idna_adapter v1.2.0
Downloaded webpki-roots v0.26.8
Downloaded icu_collections v1.5.0
Downloaded zerovec-derive v0.10.3
- cargo check --workspace: ok
solana-rent-collector v2.3.0
Checking solana-udp-client v2.2.4
Checking solana-offchain-message v2.2.1
Checking solana-rent-debits v2.2.1
Compiling anchor-attribute-program v0.31.0
Checking solana-system-transaction v2.2.1
Checking solana-nonce-account v2.2.1
Checking solana-presigner v2.2.1
Checking solana-compute-budget-interface v2.2.1
Checking colorchoice v1.0.0
Checking anstyle v1.0.10
Checking option-ext v0.2.0
Checking is_terminal_polyfill v1.70.1
Checking anstyle-query v1.1.2
Checking solana-validator-exit v2.2.1
Checking solana-rpc-client-api v2.2.4
Checking dirs-sys v0.4.1
Checking clap_lex v0.6.0
Checking anstream v0.6.18
Compiling heck v0.4.1
Compiling ruint-macro v1.2.1
Compiling rust_decimal v1.37.1
Compiling clap_derive v4.4.7
Checking clap_builder v4.4.7
Checking ruint v1.9.0
Checking dirs v5.0.1
Compiling bigdecimal v0.4.7
Checking shellexpand v3.1.0
Checking webpki-roots v0.26.8
Checking ureq v2.12.1
Checking solana-transaction-status v2.2.4
Checking routerify v3.0.0
Checking anchor-lang v0.31.0
Checking serde_json_any_key v2.0.0
Checking solana-rpc-client v2.2.4
Checking solana-pubsub-client v2.2.4
Checking solana-quic-client v2.2.4
Checking anchor-spl v0.31.0
Checking clap v4.4.7
Checking solana-thin-client v2.2.4
Checking solana-tpu-client v2.2.4
Checking solana-rpc-client-nonce-utils v2.2.4
Checking ark-poly v0.4.2
Checking solana-client v2.2.4
Checking ark-ec v0.4.2
Checking ark-bn254 v0.4.0
Checking solana-bn254 v2.2.1
Checking solana-sdk v2.2.1
Checking anchor-client v0.31.0
Checking commons v0.3.2 (/data/workdir/MeteoraAg/dlmm-sdk/commons)
Checking cli v0.5.2 (/data/workdir/MeteoraAg/dlmm-sdk/cli)
Checking market_making v0.0.1 (/data/workdir/MeteoraAg/dlmm-sdk/market_making)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 52.64s
- cargo fmt --check: ok
- cargo clippy --workspace --all-targets -- -D warnings: failed
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `bin_array_bitmap >>= bin_array_offset`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern
error: manual implementation of an assign operation
--> commons/src/extensions/bin_array_bitmap.rs:191:25
|
191 | bin_array_bitmap = bin_array_bitmap >> bin_array_offset;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `bin_array_bitmap >>= bin_array_offset`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern
error: manual implementation of an assign operation
--> commons/src/extensions/bin_array_bitmap.rs:221:25
|
221 | / bin_array_bitmap = bin_array_bitmap
222 | | << BIN_ARRAY_BITMAP_SIZE as usize - bin_array_offset - 1;
| |____________________________________________________________________________________^ help: replace it with: `bin_array_bitmap <<= BIN_ARRAY_BITMAP_SIZE as usize - bin_array_offset - 1`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern
error: casting the result of `i32::abs()` to u32
--> commons/src/math/u64x64_math.rs:29:32
|
29 | let exp: u32 = if invert { exp.abs() as u32 } else { exp as u32 };
| ^^^^^^^^^^^^^^^^ help: replace with: `exp.unsigned_abs()`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cast_abs_to_unsigned
= note: `-D clippy::cast-abs-to-unsigned` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::cast_abs_to_unsigned)]`
error: could not compile `commons` (lib) due to 23 previous errors
warning: build failed, waiting for other jobs to finish...
|
/bot review |
|
Bot Agent progress:
Progress:
|
There was a problem hiding this comment.
Summary
The release sync mostly looks fine, but I found three actionable CLI issues: malformed --bins values can be silently misparsed, place_limit_order can panic on a bad keypair path, and the claim-fee-operator close command still derives the old PDA/account mapping, which likely breaks closing operator accounts created by the renamed create flow.
Inline comments
See inline comments on the diff.
Claude (API) review
Rounds: 2 (min=1, max=10).
First look: Codex + Claude (Claude first-look failed; fell back to Codex).
Agreement: yes (no new findings/questions).
Debate transcript
First look comparison
Codex (first look):
Summary:
The release sync mostly looks fine, but I found three actionable CLI issues: malformed --bins values can be silently misparsed, place_limit_order can panic on a bad keypair path, and the renamed claim-fee-operator create path no longer matches the similarly named close path unless both PDA helpers intentionally alias the same seeds.
Inline comment highlights:
- cli/src/args.rs:66:
parse_bin_limit_ordernever checks that the input contains exactly two comma-separated fields. Because it justpop()s the last two entries, a value like1,2,3will be accepted as(2, 3)instead of being rejected, which can place an - cli/src/instructions/place_limit_order.rs:42: This
expect(...)turns a bad or unreadable--limit-order-keypair-pathinto a panic. Since the command already returnsResult<()>, it would be safer to propagate theread_keypair_fileerror with context so user input mistakes produce - cli/src/instructions/admin/create_claim_protocol_fee_operator.rs:16: This command now creates
CreateOperatorAccountatderive_operator_pda(operator), butclose_claim_protocol_fee_operatorstill derivesderive_claim_protocol_fee_operator_pda(operator)and closes that older PDA type. Unless those helpe
Codex (first look fallback):
Summary:
Found three actionable issues: the claim-fee operator close path appears out of sync with the new generic operator PDA flow, the new --bins parser can silently accept malformed input, and the CLI changelog version does not match the crate version bump.
Inline comment highlights:
- cli/src/instructions/admin/close_claim_protocol_fee_operator.rs:16: This still derives the account with
derive_claim_protocol_fee_operator_pda(...), but the create path in this PR was switched toderive_operator_pda(...)(create_claim_protocol_fee_operator.rs) andwithdraw_protocol_feenow also uses - cli/src/args.rs:66: Please validate that the input has exactly two comma-separated fields before popping values. As written, malformed input like
--bins 100,200,300is accepted and interpreted as(200, 300), silently discarding the leading segment. For an - CHANGELOG.md:22: The changelog announces
cli [0.6.3], but this PR only bumps the Rust crate to0.5.3(cli/Cargo.tomlandCargo.lock). On a release-sync PR, that mismatch will leave release notes and the published crate version out of sync, which is
Round 1
Codex (primary):
Summary:
The release sync mostly looks fine, but I found three actionable CLI issues: malformed --bins values can be silently misparsed, place_limit_order can panic on a bad keypair path, and the renamed claim-fee-operator create path no longer matches the similarly named close path unless both PDA helpers intentionally alias the same seeds.
Inline comment highlights:
- cli/src/args.rs:66:
parse_bin_limit_ordernever checks that the input contains exactly two comma-separated fields. Because it justpop()s the last two entries, a value like1,2,3will be accepted as(2, 3)instead of being rejected, which can place an - cli/src/instructions/place_limit_order.rs:42: This
expect(...)turns a bad or unreadable--limit-order-keypair-pathinto a panic. Since the command already returnsResult<()>, it would be safer to propagate theread_keypair_fileerror with context so user input mistakes produce - cli/src/instructions/admin/create_claim_protocol_fee_operator.rs:16: This command now creates
CreateOperatorAccountatderive_operator_pda(operator), butclose_claim_protocol_fee_operatorstill derivesderive_claim_protocol_fee_operator_pda(operator)and closes that older PDA type. Unless those helpe
Claude (API):
Critique of Primary Review
The primary review correctly identified the three most impactful issues:
parse_bin_limit_ordersilent misparsing — Good catch, well-explained.place_limit_orderexpect()panic — Valid; this is inconsistent with the rest of the codebase which uses?.- PDA mismatch in close vs create operator paths — Correctly identified but could be stronger: the primary review hedged with "unless those helpers [alias the same seeds]" but given the rename from
derive_claim_protocol_fee_operator_pdatoderive_operator_pdaand the complete restructuring of the create path (new struct name, new fields, new arg type), it's overwhelmingly likely these are different PDAs. The close path is almost certainly broken for newly-created accounts.
Missed by Primary Review
Nothing significant was missed. The primary review covered the key issues well.
False Positive in Round-0
The CHANGELOG version mismatch flagged in round 0 is likely a false positive — the CHANGELOG appears to track a separate user-facing version scheme, which is common in monorepos.
Overall
The primary review is solid and focused on the right issues. The PDA mismatch finding deserves stronger language — it's not a "maybe" but a near-certain bug for the new operator flow.
Codex (response):
- New findings applied: 1/2
- False positives addressed: 0/1
- Open questions kept for author: 3/3
Round 2
Codex (primary):
Summary:
The release sync mostly looks fine, but I found three actionable CLI issues: malformed --bins values can be silently misparsed, place_limit_order can panic on a bad keypair path, and the claim-fee-operator close command still derives the old PDA/account mapping, which likely breaks closing operator accounts created by the renamed create flow.
Inline comment highlights:
- cli/src/args.rs:66:
parse_bin_limit_ordernever checks that the input contains exactly two comma-separated fields. Because it justpop()s the last two entries, a value like1,2,3will be accepted as(2, 3)instead of being rejected, which can place an - cli/src/instructions/place_limit_order.rs:42: This
expect(...)turns a bad or unreadable--limit-order-keypair-pathinto a panic. Since the command already returnsResult<()>, it would be safer to propagate theread_keypair_fileerror with context so user input mistakes produce - cli/src/instructions/admin/close_claim_protocol_fee_operator.rs:16: The create path now uses
derive_operator_pda(operator), but this close command still derivesderive_claim_protocol_fee_operator_pda(operator)and passes it through the oldclaim_fee_operatoraccount binding. That mismatch is very like
Claude (API):
Critique of Primary Review
The primary review correctly identified the three most impactful issues and they remain valid:
-
parse_bin_limit_ordersilent misparsing (args.rs:66) — Correct. Thepop()-based parsing silently accepts malformed input with extra segments. -
place_limit_orderexpect()panic (place_limit_order.rs:43) — Correct. Inconsistent with the rest of the codebase which propagates errors via?. -
PDA mismatch in close vs create operator paths (close_claim_protocol_fee_operator.rs:16) — Correct and important. The create path now uses
derive_operator_pdawhile the close path still usesderive_claim_protocol_fee_operator_pda. This was also reinforced in the prior critique round and the finding was applied.
The prior critique's additional finding about the close path was applied (1/2). The remaining items were properly kept as open questions for the author.
I have no additional findings beyond what the primary review and prior critique already cover. The review is thorough and focused on the right issues. cargo check passes, confirming no compilation errors, so the module restructuring (admin → operator) is wired correctly. The clippy and fmt failures are pre-existing style issues in commons/ unrelated to this PR's changes.
Codex (response):
Agreement reached. No further changes.
Repo checks
Repo Checks
- rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu rustfmt: ok
info: downloading component 'rustfmt'
info: installing component 'rustfmt'
- rustup component add --toolchain 1.85.0-x86_64-unknown-linux-gnu clippy: ok
info: downloading component 'clippy'
info: installing component 'clippy'
- cargo fetch --locked: ok
.8.24
Downloaded webpki-root-certs v1.0.3
Downloaded solana-compute-budget v2.2.4
Downloaded solana-type-overrides v2.2.4
Downloaded solana-reserved-account-keys v2.2.1
Downloaded solana-signature v2.2.1
Downloaded icu_provider_macros v1.5.0
Downloaded solana-compute-budget-program v2.2.4
Downloaded solana-system-program v2.2.4
Downloaded solana-poseidon v2.2.4
Downloaded solana-vote-program v2.2.4
Downloaded humantime v2.1.0
Downloaded winapi-util v0.1.5
Downloaded dirs v5.0.1
Downloaded bigdecimal v0.4.7
Downloaded serde_json_any_key v2.0.0
Downloaded anstyle-query v1.1.2
Downloaded anstyle-wincon v3.0.7
Downloaded colorchoice v1.0.0
Downloaded solana-banks-client v2.2.4
Downloaded openssl-probe v0.1.5
Downloaded schannel v0.1.22
Downloaded solana-transaction-metrics-tracker v2.2.4
Downloaded tracing-attributes v0.1.27
Downloaded utf16_iter v1.0.5
Downloaded icu_locid_transform_data v1.5.0
Downloaded litemap v0.7.4
Downloaded tinystr v0.7.6
Downloaded yoke-derive v0.7.5
Downloaded synstructure v0.13.1
Downloaded zerofrom-derive v0.1.5
Downloaded rustversion v1.0.18
Downloaded ureq v2.12.1
Downloaded ring v0.17.3
Downloaded anchor-attribute-account v0.31.0
Downloaded anchor-syn v0.31.0
Downloaded solana-rpc-client-nonce-utils v2.2.4
Downloaded solana-thin-client v2.2.4
Downloaded quinn-udp v0.5.11
Downloaded solana-instruction v2.2.1
Downloaded icu_locid_transform v1.5.0
Downloaded idna_adapter v1.2.0
Downloaded proc-macro2 v1.0.94
Downloaded solana-svm-rent-collector v2.2.4
Downloaded termcolor v1.1.3
Downloaded icu_provider v1.5.0
Downloaded displaydoc v0.2.4
Downloaded zerofrom v0.1.5
Downloaded zerovec-derive v0.10.3
Downloaded once_cell v1.21.2
Downloaded icu_properties v1.5.1
Downloaded writeable v0.5.5
Downloaded solana-genesis-config v2.2.1
Downloaded iana-time-zone v0.1.58
Downloaded darling v0.20.1
Downloaded rand v0.9.0
Downloaded quote v1.0.37
- cargo fmt --check: failed
it_order.rs:61:
let token_mint_account = rpc_client.get_account(&token_mint).await?;
let token_program = token_mint_account.owner;
- let user_token = get_associated_token_address_with_program_id(
- &program.payer(),
- &token_mint,
- &token_program,
- );
+ let user_token =
+ get_associated_token_address_with_program_id(&program.payer(), &token_mint, &token_program);
let bitmap_extension_key = derive_bin_array_bitmap_extension(lb_pair).0;
let bin_array_bitmap_extension = rpc_client
Diff in /data/workdir/MeteoraAg/dlmm-sdk/commons/src/account_filters.rs:16:
vec![position_pair_filter, position_owner_filter]
}
-pub fn limit_order_filter_by_owner_and_pair(
- owner: Pubkey,
- pair: Pubkey,
-) -> Vec<RpcFilterType> {
+pub fn limit_order_filter_by_owner_and_pair(owner: Pubkey, pair: Pubkey) -> Vec<RpcFilterType> {
let discriminator_filter =
RpcFilterType::Memcmp(Memcmp::new_base58_encoded(0, &LimitOrder::DISCRIMINATOR));
Diff in /data/workdir/MeteoraAg/dlmm-sdk/commons/src/extensions/bin.rs:102:
// swap_for_y (selling X for Y) can fill bid side orders (!is_ask_side)
// !swap_for_y (selling Y for X) can fill ask side orders (is_ask_side)
if (swap_for_y && !is_ask_side) || (!swap_for_y && is_ask_side) {
- (self.open_order_amount, self.processed_order_remaining_amount)
+ (
+ self.open_order_amount,
+ self.processed_order_remaining_amount,
+ )
} else {
(0, 0)
}
Diff in /data/workdir/MeteoraAg/dlmm-sdk/commons/src/extensions/lb_pair.rs:262:
}
fn is_support_limit_order(&self) -> bool {
- let Some(function_type) = FunctionType::try_from(self.parameters.function_type).ok()
- else {
+ let Some(function_type) = FunctionType::try_from(self.parameters.function_type).ok() else {
return false;
};
match function_type {
- cargo check --workspace: ok
ng solana-udp-client v2.2.4
Checking solana-rent-collector v2.3.0
Checking solana-account-decoder v2.2.4
Checking solana-offchain-message v2.2.1
Checking solana-rent-debits v2.2.1
Checking solana-system-transaction v2.2.1
Checking solana-nonce-account v2.2.1
Checking solana-presigner v2.2.1
Checking solana-compute-budget-interface v2.2.1
Checking option-ext v0.2.0
Checking anstyle-query v1.1.2
Checking colorchoice v1.0.0
Checking solana-validator-exit v2.2.1
Checking anstyle v1.0.10
Checking is_terminal_polyfill v1.70.1
Checking dirs-sys v0.4.1
Compiling rust_decimal v1.37.1
Checking anstream v0.6.18
Compiling ruint-macro v1.2.1
Checking ark-poly v0.4.2
Checking clap_lex v0.6.0
Compiling heck v0.4.1
Checking clap_builder v4.4.7
Checking dirs v5.0.1
Checking solana-rpc-client-api v2.2.4
Compiling bigdecimal v0.4.7
Compiling clap_derive v4.4.7
Checking ruint v1.9.0
Checking shellexpand v3.1.0
Checking webpki-roots v0.26.8
Checking ureq v2.12.1
Checking ark-ec v0.4.2
Checking solana-transaction-status v2.2.4
Checking anchor-lang v0.31.0
Checking routerify v3.0.0
Checking serde_json_any_key v2.0.0
Checking anchor-spl v0.31.0
Checking solana-rpc-client v2.2.4
Checking solana-pubsub-client v2.2.4
Checking solana-quic-client v2.2.4
Checking clap v4.4.7
Checking ark-bn254 v0.4.0
Checking solana-bn254 v2.2.1
Checking solana-rpc-client-nonce-utils v2.2.4
Checking solana-tpu-client v2.2.4
Checking solana-thin-client v2.2.4
Checking solana-sdk v2.2.1
Checking solana-client v2.2.4
Checking anchor-client v0.31.0
Checking commons v0.3.3 (/data/workdir/MeteoraAg/dlmm-sdk/commons)
Checking market_making v0.0.1 (/data/workdir/MeteoraAg/dlmm-sdk/market_making)
Checking cli v0.5.3 (/data/workdir/MeteoraAg/dlmm-sdk/cli)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 56.71s
- cargo clippy --workspace --all-targets -- -D warnings: failed
formation visit https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern
error: manual implementation of an assign operation
--> commons/src/extensions/bin_array_bitmap.rs:221:25
|
221 | / bin_array_bitmap = bin_array_bitmap
222 | | << BIN_ARRAY_BITMAP_SIZE as usize - bin_array_offset - 1;
| |____________________________________________________________________________________^ help: replace it with: `bin_array_bitmap <<= BIN_ARRAY_BITMAP_SIZE as usize - bin_array_offset - 1`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern
error: casting the result of `i32::abs()` to u32
--> commons/src/math/u64x64_math.rs:29:32
|
29 | let exp: u32 = if invert { exp.abs() as u32 } else { exp as u32 };
| ^^^^^^^^^^^^^^^^ help: replace with: `exp.unsigned_abs()`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cast_abs_to_unsigned
= note: `-D clippy::cast-abs-to-unsigned` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::cast_abs_to_unsigned)]`
error: this expression creates a reference which is immediately dereferenced by the compiler
--> commons/src/account_filters.rs:24:61
|
24 | RpcFilterType::Memcmp(Memcmp::new_base58_encoded(0, &LimitOrder::DISCRIMINATOR));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `LimitOrder::DISCRIMINATOR`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow
= note: `-D clippy::needless-borrow` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::needless_borrow)]`
error: could not compile `commons` (lib) due to 24 previous errors
warning: build failed, waiting for other jobs to finish...
There was a problem hiding this comment.
[R] I think we're missing a method to support the new add_liquidity_by_weight2 endpoint
codewithgun
force-pushed
the
sync/release-0.12.0
branch
from
faa5e07 to
f778db6
Compare
| ### Added | ||
|
|
||
| - Added `concreteFunctionType`, `collectFeeMode` parameters in `createCustomizablePermissionlessLbPair` function. This allow user to create pair with function of LimitOrder or LiquidityMining and collect fee in InputToken, or TokenY (single sided fee). | ||
| - Added `concreteFunctionType`, `collectFeeMode` parameters in `getPairPubkeyIfExists` function. This allow user to query existing pair with addition filter of function type and collect fee mode. |
There was a problem hiding this comment.
@codewithgun can put this in breaking change? tqtq
* feat/add quoteCreateLimitOrder * fix: use bin array and relative bins. same as placeLimitOrder * fix: add doc and fix tyoe * fix: limit order bin array (#278) * fix: limit order bin array * fix: fetch bitmap extension from onchain during quote --------- Co-authored-by: kampung-tech <codewithgun@gmail.com>
4 participants
No description provided.