feat: support USDC on order book functionality create market#1304
feat: support USDC on order book functionality create market#1304
Conversation
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a tn_utils precompile Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test / Client
participant Engine as Kwil Engine
participant Precompile as tn_utils.precompile<br/>compute_attestation_hash
participant DB as Database (ob_queries, markets)
Test->>Test: ABI-encode query_components
Test->>Engine: create_market(bridge, query_components, ...)
Engine->>Precompile: compute_attestation_hash(query_components)
Precompile->>Precompile: ABI-unpack (address, bytes32, string, bytes)
Precompile->>Precompile: Map action string -> uint16
Precompile->>Precompile: Build length-prefixed payload (ver, algo, fields)
Precompile->>Precompile: SHA-256(payload) -> 32 bytes
Precompile-->>Engine: return 32-byte hash
Engine->>DB: check duplicate market by hash
DB-->>Engine: OK / Duplicate error
Engine->>DB: INSERT ob_queries (query_components) and markets (hash, bridge, ...)
DB-->>Engine: confirmation
Engine-->>Test: success / error response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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
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://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@tests/streams/order_book/buy_order_test.go`:
- Around line 87-217: The test contains a large DEBUG block that queries DB
internals (calls to app.Engine.ExecuteWithoutEngineCtx,
testerc20.GetUserBalance, getBalance/getUSDCBalance and many t.Logf lines) which
must be removed or gated before merge; either delete the entire DEBUG section
(all calls that log reward_instances, balances, pending_data, topics, logs and
related counts/rows) or wrap it behind a clear test flag/verbose switch (e.g.,
an isDebugTest boolean) so it only runs when explicitly enabled and does not
execute in regular CI runs.
In `@tests/streams/order_book/create_market_with_components_test.go`:
- Line 81: The test streamID string literals exceed 32 bytes and will be
truncated when copied into the [32]byte in encodeQueryComponentsABI, risking
collisions; update each test's streamID literal (the variables named streamID
used in create_market_with_components_test.go) so they are exactly 32 characters
long and remain unique (either remove or add characters to reach 32 chars) and
re-run tests to ensure no unintended truncation occurs.
In `@tests/streams/order_book/hash_compatibility_test.go`:
- Around line 328-329: The stream ID literals streamID1 and streamID2 in the
hash_compatibility_test (variables streamID1 and streamID2) are only 30
characters; update them to be exactly 32 characters (e.g., append two characters
or change the suffix) so the test matches the "exactly 32 characters"
expectation and avoids any padding/hash mismatches, and adjust the surrounding
comment if present to confirm 32-char requirement.
In `@tests/streams/order_book/market_creation_test.go`:
- Around line 51-55: The package-level counters trufPointCounter and
usdcPointCounter are unsafe for parallel tests because giveBalance increments
them; replace them with concurrency-safe counters (e.g., use sync/atomic
operations via atomic.Int64 or int64 variables updated with atomic.AddInt64) or
convert them to test-local state passed into giveBalance so each test controls
its own counter; update references to trufPointCounter++/usdcPointCounter++ in
giveBalance and tests to use atomic.AddInt64(&trufPointCounter, 1) or the
test-local increment function, and ensure giveBalance signature or call sites
are adjusted to accept the per-test counter if you choose the test-local
approach.
- Around line 334-349: The stream ID padding logic uses string(make([]byte,
...)) which inserts null bytes and then truncates silently; update the
construction of streamID used before encodeQueryComponents and callCreateMarket
so it builds a short base (e.g., "stlistmarkets%d"), then pad with a printable
filler (like '0' or spaces) using strings.Repeat to exactly 32 characters and
slice safely: create base := fmt.Sprintf("stlistmarkets%d", i), then streamID :=
(base + strings.Repeat("0", 32))[:32]; replace the existing streamID
creation/trim logic with this to avoid null bytes and accidental data loss when
encoding/querying.
🧹 Nitpick comments (13)
extensions/tn_utils/precompiles.go (2)
583-598: Consider whetherIsTable: trueis intentional.The method returns
IsTable: truebut only ever returns a single hash value. While this works, it's worth confirming this is the intended behavior rather thanIsTable: falsewith a single return field.
612-635: Consider caching ABI type definitions for performance.The ABI type definitions (
addressType,bytes32Type, etc.) are recreated on every handler invocation. Since these are immutable, consider defining them once at package level to avoid repeated allocations.♻️ Suggested optimization
// Package-level ABI argument definition (computed once) var queryComponentsABIArgs gethAbi.Arguments func init() { addressType, _ := gethAbi.NewType("address", "", nil) bytes32Type, _ := gethAbi.NewType("bytes32", "", nil) stringType, _ := gethAbi.NewType("string", "", nil) bytesType, _ := gethAbi.NewType("bytes", "", nil) queryComponentsABIArgs = gethAbi.Arguments{ {Type: addressType, Name: "data_provider"}, {Type: bytes32Type, Name: "stream_id"}, {Type: stringType, Name: "action_id"}, {Type: bytesType, Name: "args"}, } }Then in the handler, simply use:
- // Define ABI type for query components: (address, bytes32, string, bytes) - addressType, err := gethAbi.NewType("address", "", nil) - // ... (all type definitions) - args := gethAbi.Arguments{...} - - // Decode ABI - decoded, err := args.Unpack(queryComponents) + // Decode ABI using pre-defined arguments + decoded, err := queryComponentsABIArgs.Unpack(queryComponents)internal/migrations/030-order-book-schema.sql (1)
95-97: Consider deferring this index until query patterns are established.The comment states this index is for "potential future queries." Indexes on BYTEA columns can be expensive and are typically only useful for exact-match lookups. Consider adding this index when concrete query patterns justify it.
tests/streams/order_book/test_helpers_orderbook.go (2)
16-72: Code duplication withencodeQueryComponentsin market_creation_test.go.This function largely duplicates
encodeQueryComponentsdefined inmarket_creation_test.go(lines 611-650). The only difference is the default args handling (lines 17-32). Consider consolidating these into a single helper to avoid maintenance burden.♻️ Suggested consolidation
Either:
- Move the shared ABI encoding logic to a single location and have both call sites use it
- Or make
encodeQueryComponentsForTeststhe canonical implementation and remove the duplicate frommarket_creation_test.go// In test_helpers_orderbook.go - keep this as the canonical version // In market_creation_test.go - replace encodeQueryComponents calls with encodeQueryComponentsForTests
62-65: Stream ID silently truncated if longer than 32 bytes.If
streamIDexceeds 32 characters, it will be silently truncated. While test stream IDs appear carefully sized, adding validation would prevent subtle bugs.♻️ Optional: Add length validation
// Convert stream ID to bytes32 (pad with zeros on the right) var streamIDBytes [32]byte + if len(streamID) > 32 { + return nil, fmt.Errorf("streamID exceeds 32 bytes: %d", len(streamID)) + } copy(streamIDBytes[:], []byte(streamID))tests/streams/order_book/rewards_test.go (1)
740-743: Minor: Extra blank lines after encoding.Lines 741-742 have unnecessary blank lines between the encoding call and
require.NoError. While not a bug, it's inconsistent with other tests in the file.♻️ Remove extra blank lines
queryComponents, err := encodeQueryComponentsForTests(user1.Address(), "sttest00000000000000000000000044", "get_record", []byte{0x01}) - - - require.NoError(t, err)tests/streams/order_book/market_creation_test.go (1)
609-650: Duplicate ofencodeQueryComponentsForTests.This
encodeQueryComponentsfunction is nearly identical toencodeQueryComponentsForTestsintest_helpers_orderbook.go. Consider removing this duplicate and using the shared helper instead.tests/streams/order_book/hash_compatibility_test.go (2)
59-62: Global mutable state may cause test isolation issues.The package-level variables
hashCompatBalanceCounterandhashCompatLastPointare shared across tests. While you resethashCompatLastPointat the start of each test function, if tests are ever run in parallel (e.g., viat.Parallel()), this could cause race conditions or flaky tests.Consider encapsulating this state within a test helper struct passed to each test, or document that these tests must not run in parallel.
479-514: Duplicate ABI encoding helper function.This function
encodeQueryComponentsABIHashCompatis nearly identical toencodeQueryComponentsABIincreate_market_with_components_test.goand possiblyencodeQueryComponentsForTestsintest_helpers_orderbook.go. Consider consolidating into a single shared helper to reduce duplication and ensure consistency.tests/streams/order_book/create_market_with_components_test.go (3)
43-46: Global mutable state for balance tracking.Same concern as the other test file - these package-level variables could cause issues if tests run in parallel. The comment notes it uses a different starting counter (200) to avoid conflicts, which helps when running tests sequentially but doesn't prevent race conditions in parallel execution.
84-84: Inconsistent helper function usage.This test uses
encodeQueryComponentsForTestswhile all other tests in this file use the localencodeQueryComponentsABIfunction. If these helpers have even slightly different implementations, tests could pass/fail inconsistently.Consider using a single helper consistently throughout the file.
241-243: Missing assertion for error message content.The comment indicates the error should mention "decoding or invalid components", but this is only logged, not asserted. Consider adding an assertion to verify the error message provides meaningful feedback.
require.Error(t, err, "malformed ABI should be rejected") - // Error should mention decoding or invalid components - t.Logf("Malformed ABI error: %v", err) + // Verify error message is meaningful + require.True(t, + strings.Contains(err.Error(), "decode") || + strings.Contains(err.Error(), "invalid") || + strings.Contains(err.Error(), "components"), + "error should mention decoding or invalid components: %v", err)internal/migrations/032-order-book-actions.sql (1)
278-280: Documentation references outdated hash mechanism.The comment mentions "SHA256 hash" but the implementation now computes hashes via
tn_utils.compute_attestation_hash. Consider updating the comment to reflect the current mechanism.- * - $query_hash: The SHA256 hash of the market's attestation query + * - $query_hash: The attestation hash of the market's query components
| // DEBUG: Check balances AFTER initialization | ||
| trufBal, err := getBalance(ctx, platform, userAddr.Address()) | ||
| require.NoError(t, err) | ||
| usdcBal, err := getUSDCBalance(ctx, platform, userAddr.Address()) | ||
| require.NoError(t, err) | ||
| t.Logf("DEBUG: After init - TRUF: %s (expected 100e18), USDC: %s (expected 100e18)", trufBal.String(), usdcBal.String()) | ||
|
|
||
| // DEBUG: Try to get balance directly from DB to see if it's there | ||
| balanceStr, err := testerc20.GetUserBalance(ctx, platform, testUSDCExtensionName, userAddr.Address()) | ||
| t.Logf("DEBUG: USDC balance from GetUserBalance: %s (err: %v)", balanceStr, err) | ||
|
|
||
| // DEBUG: Check what extension instances exist | ||
| app := &common.App{DB: platform.DB, Engine: platform.Engine} | ||
| var instanceCount int | ||
| _ = app.Engine.ExecuteWithoutEngineCtx(ctx, app.DB, `{kwil_erc20_meta}SELECT COUNT(*) FROM reward_instances`, nil, func(r *common.Row) error { | ||
| instanceCount = int(r.Values[0].(int64)) | ||
| return nil | ||
| }) | ||
| t.Logf("DEBUG: Total ERC20 instances in DB: %d", instanceCount) | ||
|
|
||
| // DEBUG: List all instances with sync status | ||
| _ = app.Engine.ExecuteWithoutEngineCtx(ctx, app.DB, `{kwil_erc20_meta} | ||
| SELECT escrow_address, synced, erc20_address, erc20_decimals | ||
| FROM reward_instances`, nil, func(r *common.Row) error { | ||
| escrow := r.Values[0].([]byte) | ||
| synced := r.Values[1].(bool) | ||
| var erc20Addr []byte | ||
| if r.Values[2] != nil { | ||
| erc20Addr = r.Values[2].([]byte) | ||
| } | ||
| var decimals *int64 | ||
| if r.Values[3] != nil { | ||
| d := r.Values[3].(int64) | ||
| decimals = &d | ||
| } | ||
| t.Logf("DEBUG: Instance escrow=0x%x synced=%v erc20=0x%x decimals=%v", | ||
| escrow, synced, erc20Addr, decimals) | ||
| return nil | ||
| }) | ||
|
|
||
| // DEBUG: Check user balances table (correct table is 'balances', column is 'address') | ||
| var balanceCount int | ||
| _ = app.Engine.ExecuteWithoutEngineCtx(ctx, app.DB, `{kwil_erc20_meta}SELECT COUNT(*) FROM balances WHERE address = $addr`, | ||
| map[string]any{"$addr": []byte(userAddr.Bytes())}, func(r *common.Row) error { | ||
| balanceCount = int(r.Values[0].(int64)) | ||
| return nil | ||
| }) | ||
| t.Logf("DEBUG: User has %d balance records in balances table", balanceCount) | ||
|
|
||
| // DEBUG: Show actual balance values for this user | ||
| _ = app.Engine.ExecuteWithoutEngineCtx(ctx, app.DB, `{kwil_erc20_meta} | ||
| SELECT b.balance, r.escrow_address, r.erc20_address | ||
| FROM balances b | ||
| JOIN reward_instances r ON b.reward_id = r.id | ||
| WHERE b.address = $addr`, | ||
| map[string]any{"$addr": []byte(userAddr.Bytes())}, func(r *common.Row) error { | ||
| balance := r.Values[0].(*types.Decimal).String() | ||
| escrow := "0x" + hex.EncodeToString(r.Values[1].([]byte)) | ||
| erc20 := "0x" + hex.EncodeToString(r.Values[2].([]byte)) | ||
| t.Logf("DEBUG: Balance=%s escrow=%s erc20=%s", balance, escrow, erc20) | ||
| return nil | ||
| }) | ||
|
|
||
| // DEBUG: Check if ordered-sync namespace exists | ||
| var namespaceExists bool | ||
| err = app.Engine.ExecuteWithoutEngineCtx(ctx, app.DB, `SELECT 1`, nil, func(r *common.Row) error { | ||
| namespaceExists = true | ||
| return nil | ||
| }) | ||
| t.Logf("DEBUG: Can execute queries: %v (err: %v)", namespaceExists, err) | ||
|
|
||
| // DEBUG: Check if ordered-sync pending_data were stored | ||
| var pendingCount int | ||
| err = app.Engine.ExecuteWithoutEngineCtx(ctx, app.DB, `{kwil_ordered_sync}SELECT COUNT(*) FROM pending_data`, nil, func(r *common.Row) error { | ||
| pendingCount = int(r.Values[0].(int64)) | ||
| return nil | ||
| }) | ||
| t.Logf("DEBUG: Total ordered-sync pending_data: %d (err: %v)", pendingCount, err) | ||
|
|
||
| // DEBUG: Check topics table | ||
| var topicCount int | ||
| err = app.Engine.ExecuteWithoutEngineCtx(ctx, app.DB, `{kwil_ordered_sync}SELECT COUNT(*) FROM topics`, nil, func(r *common.Row) error { | ||
| topicCount = int(r.Values[0].(int64)) | ||
| return nil | ||
| }) | ||
| t.Logf("DEBUG: Total ordered-sync topics: %d (err: %v)", topicCount, err) | ||
|
|
||
| // DEBUG: List pending_data points with topic info | ||
| _ = app.Engine.ExecuteWithoutEngineCtx(ctx, app.DB, `{kwil_ordered_sync} | ||
| SELECT p.point, p.previous_point, t.name, t.last_processed_point | ||
| FROM pending_data p | ||
| JOIN topics t ON p.topic_id = t.id | ||
| ORDER BY p.point`, nil, func(r *common.Row) error { | ||
| point := r.Values[0].(int64) | ||
| var prevPoint *int64 | ||
| if r.Values[1] != nil { | ||
| p := r.Values[1].(int64) | ||
| prevPoint = &p | ||
| } | ||
| topicName := r.Values[2].(string) | ||
| var lastProcessed *int64 | ||
| if r.Values[3] != nil { | ||
| lp := r.Values[3].(int64) | ||
| lastProcessed = &lp | ||
| } | ||
| var lpStr string | ||
| if lastProcessed != nil { | ||
| lpStr = fmt.Sprintf("%d", *lastProcessed) | ||
| } else { | ||
| lpStr = "nil" | ||
| } | ||
| var prevStr string | ||
| if prevPoint != nil { | ||
| prevStr = fmt.Sprintf("%d", *prevPoint) | ||
| } else { | ||
| prevStr = "nil" | ||
| } | ||
| t.Logf("DEBUG: pending point=%d prev=%s topic=%s last_processed=%s MATCH=%v", | ||
| point, prevStr, topicName, lpStr, (prevPoint == nil && lastProcessed == nil) || (prevPoint != nil && lastProcessed != nil && *prevPoint == *lastProcessed)) | ||
| return nil | ||
| }) | ||
|
|
||
| // DEBUG: Check finalized status | ||
| _ = app.Engine.ExecuteWithoutEngineCtx(ctx, app.DB, `{kwil_ordered_sync}SELECT topic, point, finalized FROM logs ORDER BY point`, nil, func(r *common.Row) error { | ||
| topic := r.Values[0].(string) | ||
| point := r.Values[1].(int64) | ||
| finalized := r.Values[2].(bool) | ||
| t.Logf("DEBUG: Log topic=%s point=%d finalized=%v", topic, point, finalized) | ||
| return nil | ||
| }) | ||
|
|
There was a problem hiding this comment.
Remove extensive DEBUG logging before merge.
This block contains ~130 lines of debug code that queries internal tables (reward_instances, balances, pending_data, topics, logs), checks singleton state, and logs detailed internal state. While useful during development, this verbose debugging should be removed or gated behind a test flag before merging.
🧹 Suggested cleanup
Either:
- Remove the DEBUG blocks entirely
- Or wrap them in a conditional:
+const debugBuyOrderTests = false // Set to true for debugging
// In testBuyOrderSuccessful:
+if debugBuyOrderTests {
// DEBUG: Check balances AFTER initialization
trufBal, err := getBalance(ctx, platform, userAddr.Address())
// ... rest of debug code ...
+}🤖 Prompt for AI Agents
In `@tests/streams/order_book/buy_order_test.go` around lines 87 - 217, The test
contains a large DEBUG block that queries DB internals (calls to
app.Engine.ExecuteWithoutEngineCtx, testerc20.GetUserBalance,
getBalance/getUSDCBalance and many t.Logf lines) which must be removed or gated
before merge; either delete the entire DEBUG section (all calls that log
reward_instances, balances, pending_data, topics, logs and related counts/rows)
or wrap it behind a clear test flag/verbose switch (e.g., an isDebugTest
boolean) so it only runs when explicitly enabled and does not execute in regular
CI runs.
| var ( | ||
| twoTRUF = mustParseBigInt(marketFee) | ||
| pointCounter int64 = 100 // Start from 100 to avoid conflicts with other tests | ||
| twoTRUF = mustParseBigInt(marketFee) | ||
| trufPointCounter int64 = 100 // Counter for TRUF bridge (hoodi_tt) | ||
| usdcPointCounter int64 = 10000 // Counter for USDC bridge (hoodi_tt2) - far apart to avoid conflicts | ||
| ) |
There was a problem hiding this comment.
Global point counters may cause issues with parallel test execution.
trufPointCounter and usdcPointCounter are package-level variables that are incremented in giveBalance. If tests run in parallel (e.g., with t.Parallel()), this could cause race conditions or unexpected point values.
🔧 Suggested fix: Use atomic counters or test-local state
var (
twoTRUF = mustParseBigInt(marketFee)
- trufPointCounter int64 = 100 // Counter for TRUF bridge (hoodi_tt)
- usdcPointCounter int64 = 10000 // Counter for USDC bridge (hoodi_tt2) - far apart to avoid conflicts
+ trufPointCounter atomic.Int64 // Counter for TRUF bridge (hoodi_tt)
+ usdcPointCounter atomic.Int64 // Counter for USDC bridge (hoodi_tt2)
)
+
+func init() {
+ trufPointCounter.Store(100)
+ usdcPointCounter.Store(10000)
+}And use trufPointCounter.Add(1) instead of trufPointCounter++.
🤖 Prompt for AI Agents
In `@tests/streams/order_book/market_creation_test.go` around lines 51 - 55, The
package-level counters trufPointCounter and usdcPointCounter are unsafe for
parallel tests because giveBalance increments them; replace them with
concurrency-safe counters (e.g., use sync/atomic operations via atomic.Int64 or
int64 variables updated with atomic.AddInt64) or convert them to test-local
state passed into giveBalance so each test controls its own counter; update
references to trufPointCounter++/usdcPointCounter++ in giveBalance and tests to
use atomic.AddInt64(&trufPointCounter, 1) or the test-local increment function,
and ensure giveBalance signature or call sites are adjusted to accept the
per-test counter if you choose the test-local approach.
|
Log: the patch is too large, will work the rest on the next PRs |
1 participant
resolves: https://github.com/truflation/website/issues/3205
Summary by CodeRabbit
New Features
Behavior Changes
Database
Tests
✏️ Tip: You can customize this high-level summary in your review settings.