chore: enable SDK functionality for primitive stream#164
Conversation
WalkthroughAdds transport-backed IAction implementations (TransportAction and TransportPrimitiveAction), implements GetRecord and primitive InsertRecords over a generic Transport, updates client branching to select HTTP vs generic Transport paths, and centralizes GetRecordRawOutput into core/types for shared decoding. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TransportAction
participant Transport
participant RemoteService as Remote Service
rect rgb(223,243,255)
Note over Client,TransportAction: Read flow — GetRecord
Client->>TransportAction: GetRecord(ctx, input)
TransportAction->>TransportAction: Build args (stream, index, options via util.TransformOrNil)
TransportAction->>Transport: Call(ctx, prefix+"get_record", args)
Transport->>RemoteService: transport-level request (HTTP/RPC)
RemoteService-->>Transport: JSON result ([]types.GetRecordRawOutput)
Transport-->>TransportAction: raw result
TransportAction->>TransportAction: Decode, convert Value→apd.Decimal, parse EventTime
TransportAction-->>Client: ActionResult (typed records)
end
rect rgb(233,255,233)
Note over Client,TransportAction: Write flow — InsertRecords (primitive)
Client->>TransportAction: InsertRecords(ctx, inputs, opts)
TransportAction->>TransportAction: Convert numerics → 36/18 strings, assemble batch payload
TransportAction->>Transport: Execute(ctx, "insert_records", payload)
Transport->>RemoteService: execution/tx request
RemoteService-->>Transport: tx hash
Transport-->>TransportAction: tx hash
TransportAction-->>Client: types.Hash
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (2)core/contractsapi/stream_procedures.go (2)
core/tnclient/actions_transport.go (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (11)
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 |
Time Submission Status
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
core/tnclient/client.go (1)
135-147: Potential code duplication: Address derivation logic is repeated.The address derivation logic using
auth.EthSecp256k1Authenticator{}.Identifier(c.transport.Signer().CompactID())appears here and also in theAddress()method (lines 199-210). Consider extracting this to a helper or reusingAddress()directly.🔎 Suggested simplification
func (c *Client) DestroyStream(ctx context.Context, streamId util.StreamId) (types.Hash, error) { - // Get address from signer - addr, err := auth.EthSecp256k1Authenticator{}.Identifier(c.transport.Signer().CompactID()) - if err != nil { - return types.Hash{}, errors.WithStack(err) - } - // Use transport.Execute directly - works with all transports (HTTP, CRE, etc.) return c.transport.Execute(ctx, "", "delete_stream", [][]any{{ - addr, + c.Address().Address(), streamId.String(), }}) }Note: This assumes that if
Address()panics, the client was misconfigured at construction time. If you prefer explicit error handling here, the current approach is fine.core/tnclient/options_cre.go (1)
58-81: Placeholder methods useerrorspackage but it's imported (however unused for Signer).The placeholder methods correctly panic to indicate misconfiguration, but
Signer()has a special case that returnsp.signerif set. However, looking at line 43-46, the placeholder never setssigner- it's always nil sinceWithCRETransportdoesn't populate it.Consider either:
- Removing the
signerfield from placeholder if it's never used, or- Having
WithSigneralso set the signer on the placeholder if one existscore/tnclient/transport_cre.go (3)
56-61: TODO comment left for TxOpt handling.The TODO indicates that transaction options are not properly handled. This could affect functionality if callers pass nonce or fee options.
Would you like me to help implement proper TxOpt handling, or should this be tracked as a follow-up issue?
146-182: WaitTx doesn't check HTTP status codes or handle transient errors.The polling loop returns immediately on any HTTP error without distinguishing between transient failures (network issues, 503) and permanent failures. Consider adding retry logic for transient errors and checking
response.StatusCode.Also, the first poll waits for the full interval before checking - consider an immediate first check.
🔎 Suggested improvement for resilience
func (t *CRETransport) WaitTx(ctx context.Context, txHash kwilTypes.Hash, interval time.Duration) (*kwilTypes.TxQueryResponse, error) { - // Poll for transaction status ticker := time.NewTicker(interval) defer ticker.Stop() + // Check immediately first, then poll + checkTx := func() (*kwilTypes.TxQueryResponse, bool, error) { + req := &http.Request{ + Method: "GET", + URL: fmt.Sprintf("%s/api/v1/tx/%s", t.endpoint, txHash.String()), + } + promise := t.client.SendRequest(req) + response, err := promise.Await() + if err != nil { + // Return false to continue polling on transient errors + return nil, false, nil + } + var result kwilTypes.TxQueryResponse + if err := json.Unmarshal(response.Body, &result); err != nil { + return nil, false, errors.Wrap(err, "failed to parse tx query response") + } + if result.Hash != nil { + return &result, true, nil + } + return nil, false, nil + } + + // Immediate first check + if result, found, err := checkTx(); err != nil { + return nil, err + } else if found { + return result, nil + } + for { select { case <-ctx.Done(): return nil, ctx.Err() case <-ticker.C: - // Query transaction status - ... + if result, found, err := checkTx(); err != nil { + return nil, err + } else if found { + return result, nil + } } } }
184-210: ChainID silently swallows errors and returns empty string.While this may be intentional for resilience, it could mask configuration or network issues. Consider logging the error or providing a separate method that returns errors.
core/tnclient/actions_transport.go (2)
666-674: Consider pre-allocating with capacity hint.Line 666 creates a slice without capacity, but you already know the maximum size from
existsResults. This is a minor optimization.🔎 Suggested improvement
- filtered := make([]clientType.StreamLocator, 0) + filtered := make([]clientType.StreamLocator, 0, len(existsResults))
1449-1480:parseFeeDistributionssilently skips malformed entries.If a distribution entry is malformed (missing colon, empty parts), it's silently skipped. This may be intentional for resilience, but consider logging a warning for debugging purposes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/tnclient/actions_transport.gocore/tnclient/client.gocore/tnclient/options_cre.gocore/tnclient/transport_cre.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-10T15:39:48.576Z
Learnt from: MicBun
Repo: trufnetwork/sdk-go PR: 138
File: examples/cache_example.go:21-21
Timestamp: 2025-07-10T15:39:48.576Z
Learning: For example code in the trufnetwork/sdk-go repository, the maintainer MicBun prefers to keep examples simple and focused on demonstrating functionality rather than implementing production-ready security practices like environment variables for sensitive values.
Applied to files:
core/tnclient/client.go
🧬 Code graph analysis (4)
core/tnclient/client.go (5)
core/types/tsn_client.go (1)
Client(12-50)core/util/stream_id.go (1)
StreamId(29-31)core/types/stream.go (1)
IAction(75-127)core/tnclient/actions_transport.go (1)
TransportAction(21-23)core/types/primitive_stream.go (1)
IPrimitiveAction(17-28)
core/tnclient/transport_cre.go (2)
core/tnclient/client.go (1)
Client(20-24)core/tnclient/transport.go (1)
Transport(38-95)
core/tnclient/options_cre.go (1)
core/tnclient/client.go (2)
Option(28-28)Client(20-24)
core/tnclient/actions_transport.go (7)
core/types/stream.go (1)
IAction(75-127)core/types/primitive_stream.go (1)
IPrimitiveAction(17-28)core/contractsapi/decode_call_results.go (1)
DecodeCallResult(126-210)core/types/stream_types.go (1)
StreamLocator(6-11)core/util/ethereum_address.go (2)
EthereumAddress(15-18)NewEthereumAddressFromString(20-38)core/types/role_types.go (5)
GrantRoleInput(12-16)RevokeRoleInput(19-23)AreMembersOfInput(27-31)RoleMembershipResult(43-46)RoleMember(49-53)core/contractsapi/attestation_encoding.go (1)
EncodeActionArgs(28-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (7)
core/tnclient/client.go (3)
127-133: LGTM - Clean transport abstraction for stream deployment.The refactoring to use
transport.Executedirectly provides a clean transport-agnostic approach that works across HTTP, CRE, and other transports.
149-176: LGTM - Consistent transport-aware action loading.All
Load*Actionsmethods now return transport-wrapped implementations consistently. The pattern is clean and maintainable.
214-230: No changes needed. The array parameter format[][]any{{streamIds, streamTypes}}for thecreate_streamsprocedure is correct and verified. The procedure expects two array arguments in a single batch input, which is exactly how the code implements it. The format is consistent with the Transport interface design and matches the identical implementation in contractsapi.BatchDeployStreams, both of which include explicit comments confirming this is the expected structure.core/tnclient/actions_transport.go (4)
20-26: LGTM - Clean TransportAction base type with interface verification.The compile-time interface check
var _ clientType.IAction = (*TransportAction)(nil)ensures the implementation satisfies the interface.
696-708: LGTM - InsertRecord correctly handles decimal conversion.The use of
kwilTypes.ParseDecimalExplicitwith proper precision (36, 18) ensures accurate numeric handling.
1064-1104: Good input validation with clear error messages.The validation for limit, offset, requester length, and orderBy whitelist is thorough and provides helpful error messages.
1409-1447: LGTM - Robust helper functions for column extraction.The
extractBytesColumnandextractInt64Columnhelpers provide consistent error handling with informative messages including row index and column name.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
core/tnclient/transport_cre.go (1)
48-61: Compilation error: Execute signature still doesn't match Transport interface.The signature on line 49 uses
opts ...any, but the Transport interface requiresopts ...clientType.TxOpt(see core/tnclient/transport.go:37-94). Additionally, the TODO comment at lines 58-61 shows that TxOpt handling is not implemented.🔎 Required fix
-func (t *CRETransport) Execute(ctx context.Context, namespace string, action string, inputs [][]any, opts ...any) (kwilTypes.Hash, error) { +func (t *CRETransport) Execute(ctx context.Context, namespace string, action string, inputs [][]any, opts ...kwilClientType.TxOpt) (kwilTypes.Hash, error) { // Build request payload for kwil-db execute endpoint payload := map[string]any{ "dbid": namespace, "action": action, "inputs": inputs, } - // Add optional parameters - if len(opts) > 0 { - // TODO: Handle TxOpt options properly - // For now, we'll skip them as they require more complex handling - } + // Apply transaction options to build proper request + // Note: This may require extracting nonce, fee, etc. from opts + // and adding them to the payload based on CRE API requirementsYou'll also need to add the import:
import ( "context" "encoding/json" "fmt" "time" "github.com/pkg/errors" "github.com/smartcontractkit/cre-sdk-go/capabilities/networking/http" "github.com/smartcontractkit/cre-sdk-go/cre" "github.com/trufnetwork/kwil-db/core/crypto/auth" + kwilClientType "github.com/trufnetwork/kwil-db/core/client/types" kwilTypes "github.com/trufnetwork/kwil-db/core/types" )core/tnclient/options_cre.go (1)
44-53: Placeholder is never replaced—will panic at runtime.The code sets a
creTransportPlaceholderbut past review verified thatNewClientdoes not detect or replace it with an actualCRETransport. When any transport method is called, the placeholder's panic methods (lines 65-79) will execute.You need to add logic in
NewClient(or inWithSigner) to detect the placeholder and construct the realCRETransport:// In NewClient after all options are applied: if placeholder, ok := c.transport.(*creTransportPlaceholder); ok { if c.signer == nil { return nil, errors.New("CRE transport requires a signer") } creTransport, err := NewCRETransport(placeholder.runtime, placeholder.endpoint, c.signer) if err != nil { return nil, errors.Wrap(err, "failed to create CRE transport") } c.transport = creTransport }core/tnclient/actions_transport.go (1)
325-336: Type assertion needed for bool comparison.Line 331 directly compares
result.QueryResult.Values[0][0] == false, which will fail if the backend returns a different type representation (e.g., string "false", int 0). This should use type assertion for safety.🔎 Proposed defensive fix
func (a *TransportAction) CheckStreamExists(ctx context.Context, input clientType.CheckStreamExistsInput) error { result, err := a.transport.Call(ctx, "", "stream_exists", []any{input.DataProvider, input.StreamId}) if err != nil { return errors.WithStack(err) } - if len(result.QueryResult.Values) == 0 || result.QueryResult.Values[0][0] == false { + if len(result.QueryResult.Values) == 0 || len(result.QueryResult.Values[0]) == 0 { + return errors.New("stream not found") + } + + exists, ok := result.QueryResult.Values[0][0].(bool) + if !ok || !exists { return errors.New("stream not found") } return nil }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/tnclient/actions_transport.gocore/tnclient/client.gocore/tnclient/options_cre.gocore/tnclient/transport_cre.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/tnclient/client.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-10T15:39:48.576Z
Learnt from: MicBun
Repo: trufnetwork/sdk-go PR: 138
File: examples/cache_example.go:21-21
Timestamp: 2025-07-10T15:39:48.576Z
Learning: For example code in the trufnetwork/sdk-go repository, the maintainer MicBun prefers to keep examples simple and focused on demonstrating functionality rather than implementing production-ready security practices like environment variables for sensitive values.
Applied to files:
core/tnclient/options_cre.go
🧬 Code graph analysis (2)
core/tnclient/transport_cre.go (2)
core/tnclient/client.go (1)
Client(20-24)core/tnclient/transport.go (1)
Transport(38-95)
core/tnclient/options_cre.go (1)
core/tnclient/client.go (2)
Option(28-28)Client(20-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
core/tnclient/transport_cre.go (1)
113-244: LGTM on Call, WaitTx, ChainID, and Signer implementations.The Call method properly marshals payloads and handles responses. WaitTx correctly polls with context cancellation support. ChainID and Signer are straightforward getters.
core/tnclient/actions_transport.go (2)
43-92: Excellent fix: strconv.Atoi errors now properly handled.The GetRecord, GetIndex, GetIndexChange, and GetFirstRecord methods now check and return errors from
strconv.Atoicalls (lines 81-84, 131-134, 182-185, 237-240), addressing the previous review feedback.Also applies to: 94-142, 144-193, 195-248
258-619: Transport action implementations are well-structured.The action implementations demonstrate:
- Consistent error handling with errors.WithStack
- Thorough input validation (especially in attestation and transaction methods)
- Comprehensive type checking when parsing responses
- Good use of helper functions for common parsing tasks
- Clear separation of concerns across action families
Also applies to: 641-681, 703-822, 824-911, 913-1120, 1122-1350, 1352-1425
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/tnclient/actions_transport.go (1)
325-336: Type comparison may fail for non-bool values.Line 331 compares
result.QueryResult.Values[0][0] == falsedirectly. If the backend returns a non-bool type (string "false", int 0, etc.), this comparison will always evaluate to false, potentially causing false positives.This issue was flagged in previous reviews and remains unaddressed.
🔎 Proposed defensive fix
func (a *TransportAction) CheckStreamExists(ctx context.Context, input clientType.CheckStreamExistsInput) error { result, err := a.transport.Call(ctx, "", "stream_exists", []any{input.DataProvider, input.StreamId}) if err != nil { return errors.WithStack(err) } - if len(result.QueryResult.Values) == 0 || result.QueryResult.Values[0][0] == false { + if len(result.QueryResult.Values) == 0 || len(result.QueryResult.Values[0]) == 0 { + return errors.New("stream not found") + } + + exists, ok := result.QueryResult.Values[0][0].(bool) + if !ok || !exists { return errors.New("stream not found") } return nil }
🧹 Nitpick comments (1)
core/tnclient/actions_transport.go (1)
1394-1425: Consider logging or returning warnings for malformed fee distributions.
parseFeeDistributionssilently skips malformed entries (missing colons, empty parts). While this provides resilience against bad data, it could hide data quality issues.Consider optionally logging warnings when entries are skipped to aid debugging.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/tnclient/actions_transport.go
🧰 Additional context used
🧬 Code graph analysis (1)
core/tnclient/actions_transport.go (9)
core/tnclient/transport.go (1)
Transport(38-95)core/types/stream.go (1)
IAction(75-127)core/types/primitive_stream.go (2)
IPrimitiveAction(17-28)InsertRecordInput(10-15)core/util/stream_id.go (2)
StreamId(29-31)NewStreamId(33-43)core/types/stream_types.go (1)
StreamLocator(6-11)core/types/contract_values.go (7)
StreamType(8-8)MetadataKey(19-19)AllowReadWalletKey(27-27)AllowComposeStreamKey(28-28)ComposeVisibilityKey(25-25)ReadVisibilityKey(26-26)DefaultBaseTimeKey(29-29)core/util/visibility_enum.go (2)
VisibilityEnum(9-9)NewVisibilityEnum(16-25)core/util/address_slice.go (1)
EthereumAddressesToStrings(4-10)core/types/transaction_types.go (2)
ITransactionAction(65-72)TransactionEvent(9-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
MicBun
force-pushed
the
creCompability
branch
from
3a5e897 to
3947e0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/tnclient/actions_transport.go (1)
325-336: Type-safe boolean check needed for stream existence.Line 331 compares
result.QueryResult.Values[0][0] == falsedirectly. If the backend returns a different type (string "false", int 0, etc.), this comparison will fail.The past review comment provides a defensive fix with type assertion:
exists, ok := result.QueryResult.Values[0][0].(bool) if !ok || !exists { return errors.New("stream not found") }
🧹 Nitpick comments (1)
core/tnclient/transport_cre.go (1)
213-244: Consider logging ChainID fetch failures.The
ChainIDmethod silently returns an empty string on errors (lines 224-226, 235-236). While this may be caught by downstream validation, consider logging the error for easier debugging.🔎 Optional improvement
Add logging when ChainID fetch fails:
func (t *CRETransport) ChainID() string { req := &http.Request{ Method: "GET", URL: fmt.Sprintf("%s/api/v1/chain_info", t.endpoint), } promise := t.client.SendRequest(req) response, err := promise.Await() if err != nil { + // Consider logging: log.Warn("failed to fetch chain ID", "error", err) return "" } var result struct { Data struct { ChainID string `json:"chain_id"` } `json:"data"` } if err := json.Unmarshal(response.Body, &result); err != nil { + // Consider logging: log.Warn("failed to parse chain ID response", "error", err) return "" } return result.Data.ChainID }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
core/tnclient/actions_transport.gocore/tnclient/client.gocore/tnclient/options_cre.gocore/tnclient/transport_cre.go
🚧 Files skipped from review as they are similar to previous changes (1)
- core/tnclient/options_cre.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-10T15:39:48.576Z
Learnt from: MicBun
Repo: trufnetwork/sdk-go PR: 138
File: examples/cache_example.go:21-21
Timestamp: 2025-07-10T15:39:48.576Z
Learning: For example code in the trufnetwork/sdk-go repository, the maintainer MicBun prefers to keep examples simple and focused on demonstrating functionality rather than implementing production-ready security practices like environment variables for sensitive values.
Applied to files:
core/tnclient/client.go
🧬 Code graph analysis (2)
core/tnclient/transport_cre.go (2)
core/tnclient/client.go (1)
Client(20-24)core/tnclient/transport.go (1)
Transport(38-95)
core/tnclient/actions_transport.go (12)
core/tnclient/transport.go (1)
Transport(38-95)core/types/stream.go (2)
IAction(75-127)StreamResult(45-48)core/types/primitive_stream.go (1)
IPrimitiveAction(17-28)core/types/cache_metadata.go (1)
ActionResult(42-45)core/contractsapi/decode_call_results.go (1)
DecodeCallResult(126-210)core/types/stream_types.go (1)
StreamLocator(6-11)core/types/contract_values.go (7)
StreamType(8-8)MetadataKey(19-19)AllowReadWalletKey(27-27)AllowComposeStreamKey(28-28)ComposeVisibilityKey(25-25)ReadVisibilityKey(26-26)DefaultBaseTimeKey(29-29)core/util/ethereum_address.go (2)
EthereumAddress(15-18)NewEthereumAddressFromString(20-38)core/util/visibility_enum.go (2)
VisibilityEnum(9-9)NewVisibilityEnum(16-25)core/util/address_slice.go (1)
EthereumAddressesToStrings(4-10)core/contractsapi/attestation_encoding.go (1)
EncodeActionArgs(28-59)core/types/transaction_types.go (4)
TransactionEvent(9-18)FeeDistribution(21-24)ListTransactionFeesInput(32-37)TransactionFeeEntry(51-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (14)
core/tnclient/client.go (4)
127-133: LGTM! Transport-agnostic implementation is clean.The refactoring to use
transport.Executedirectly enables multiple transport backends (HTTP, CRE, etc.) as intended by the PR objectives.
135-144: LGTM! Consistent transport-based pattern.The signer-derived address retrieval via
Address()and transport execution are correctly implemented.
146-187: LGTM! Consistent transport-backed action loading.All
Load*Actionsmethods now return transport-aware implementations, successfully abstracting away the dependency onGetKwilClientand enabling transport-agnostic operation.
211-227: LGTM! Proper validation and transport-based batch deployment.The empty input guard and array construction are correct. The nested array structure
[][]any{{streamIds, streamTypes}}properly passes two array arguments in a single batch call tocreate_streams.core/tnclient/transport_cre.go (3)
28-46: LGTM! Thorough parameter validation.The constructor properly validates all required parameters (runtime, endpoint, signer) and returns clear error messages for validation failures.
113-161: LGTM! Consistent implementation with Execute.The Call method follows the same pattern as Execute with appropriate response parsing for read-only operations.
163-211: LGTM! Proper polling implementation with context support.The WaitTx method correctly:
- Respects context cancellation
- Treats 404 as "not found yet" and continues polling
- Handles transient errors gracefully
- Validates transaction presence before returning
core/tnclient/actions_transport.go (7)
20-42: LGTM! Clean transport abstraction layer.The TransportAction structure and its basic delegation methods provide a clean abstraction over the Transport interface.
43-193: LGTM! Consistent error handling for EventTime parsing.The GetRecord, GetIndex, and GetIndexChange methods properly handle EventTime parsing errors and maintain consistent patterns throughout.
338-555: LGTM! Consistent metadata management implementation.The metadata helper methods follow uniform patterns for error handling, response decoding, and transport interaction.
557-624: LGTM! Efficient batch operations with proper validation.The empty input guard (lines 559-561) prevents unnecessary backend calls, and the filter implementation efficiently reuses
BatchStreamExists.
626-827: LGTM! Well-structured primitive and composed action implementations.The embedding pattern is clean, type validations are appropriate, and decimal parsing uses correct precision (36, 18) for numeric values.
829-1125: LGTM! Comprehensive role management and attestation implementations.The implementations include:
- Proper input validation (RequestAttestation, ListAttestations)
- Defensive nil checks (GetSignedAttestation)
- Appropriate argument encoding for attestations
- Consistent error handling throughout
1127-1430: LGTM! Robust transaction and fee retrieval implementation.The implementation excels at:
- Thorough type checking with helpful error messages including row indices
- Flexible type handling (e.g., distribution_sequence as int64 or string)
- Proper base64 decoding in extractBytesColumn
- Clear parsing of fee distributions
MicBun
changed the title
MicBun
force-pushed
the
creCompability
branch
from
3947e0e to
3bac41b
Compare
MicBun
force-pushed
the
creCompability
branch
from
3bac41b to
3244b13
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/tnclient/client.go (1)
145-150: Transport-aware pattern missing from DestroyStream and BatchDeployStreams.DestroyStream and BatchDeployStreams use
GetKwilClient()which returnsnilfor non-HTTP transports, causing nil pointer dereference when called with custom transports. LoadRoleManagementActions, LoadAttestationActions, and LoadTransactionActions already validate and return errors for nil clients, so they are safe.Consider applying the transport-aware pattern already used in DeployStream (lines 128-143) to DestroyStream and BatchDeployStreams for consistency.
🧹 Nitpick comments (1)
core/tnclient/actions_transport.go (1)
38-40: Inconsistent handling of optionalUseCacheparameter.Other optional fields (From, To, FrozenAt) use
transformOrNilwhich explicitly passesnilwhen the value is not set.UseCacheis only appended when non-nil, which changes the argument count and could cause misalignment if the backend expects positional arguments.🔎 Suggested fix for consistency
args = append(args, transformOrNil(input.FrozenAt, func(date int) any { return date })) - if input.UseCache != nil { - args = append(args, *input.UseCache) - } + args = append(args, transformOrNil(input.BaseDate, func(date int) any { return date })) + args = append(args, transformOrNil(input.UseCache, func(b bool) any { return b }))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/tnclient/actions_transport.gocore/tnclient/client.go
🧰 Additional context used
🧬 Code graph analysis (1)
core/tnclient/actions_transport.go (7)
core/types/stream.go (10)
IAction(75-127)GetRecordInput(12-21)StreamResult(45-48)GetIndexInput(23-23)GetIndexChangeInput(25-35)GetFirstRecordInput(37-43)VisibilityInput(55-58)ReadWalletInput(50-53)DefaultBaseTimeInput(65-68)StreamExistsResult(70-73)core/types/cache_metadata.go (1)
ActionResult(42-45)core/util/stream_id.go (1)
StreamId(29-31)core/contractsapi/decode_call_results.go (1)
DecodeCallResult(126-210)core/types/stream_types.go (1)
StreamLocator(6-11)core/util/visibility_enum.go (1)
VisibilityEnum(9-9)core/types/primitive_stream.go (2)
IPrimitiveAction(17-28)InsertRecordInput(10-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (7)
core/tnclient/client.go (3)
129-143: LGTM - Clean transport branching for DeployStream.The implementation correctly branches between HTTP and custom transports. The argument structure
[][]any{{streamId.String(), streamType.String()}}aligns with the pattern used elsewhere.
152-162: LGTM - Proper transport-aware LoadActions implementation.Correctly returns
TransportActionwrapper for non-HTTP transports while preserving the full-featured GatewayClient path for HTTP.
164-176: LGTM - Consistent pattern for LoadPrimitiveActions.The embedded
TransportActioncomposition is idiomatic and matches the interface hierarchy (IPrimitiveActionembedsIAction).core/tnclient/actions_transport.go (4)
63-88: LGTM - Result parsing with proper error handling.The decimal parsing and event time conversion now include proper error handling. The cache metadata omission is appropriately documented as a known limitation.
90-111: LGTM - Correct helper and functional procedure methods.The
transformOrNilgeneric is clean and reusable.ExecuteProcedureandCallProcedurecorrectly delegate to the transport rather than being stubs, enabling custom transports to execute arbitrary procedures.
196-225: LGTM - Solid InsertRecords implementation.The decimal conversion with explicit 36 precision and 18 scale is appropriate for financial values. The batch argument structure aligns with the transport.Execute pattern used in DeployStream.
227-236: LGTM - Appropriate delegation and stub.
InsertRecordcorrectly delegates toInsertRecordsfollowing DRY principles. The stub forCheckValidPrimitiveStreamis consistent with the minimal implementation scope.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/contractsapi/stream_procedures.go (1)
510-511: Useutil.TransformOrNilconsistently with other stream functions.
GetFirstRecordusesaddArgOrNullfor theAfterandFrozenAtparameters (both*intpointers), whileGetRecord,GetIndex, andGetIndexChangeuseutil.TransformOrNilfor equivalent pointer parameters. Since these field types are identical across all input structs, adopt the same pattern for consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/contractsapi/stream_procedures.gocore/tnclient/actions_transport.gocore/util/helpers.go
🧰 Additional context used
🧬 Code graph analysis (2)
core/contractsapi/stream_procedures.go (2)
core/util/helpers.go (1)
TransformOrNil(12-17)core/types/result_types.go (1)
GetRecordRawOutput(9-12)
core/tnclient/actions_transport.go (5)
core/types/stream.go (5)
GetRecordInput(12-21)StreamResult(45-48)GetIndexInput(23-23)ReadWalletInput(50-53)StreamExistsResult(70-73)core/types/cache_metadata.go (1)
ActionResult(42-45)core/util/helpers.go (1)
TransformOrNil(12-17)core/contractsapi/decode_call_results.go (1)
DecodeCallResult(126-210)core/types/result_types.go (1)
GetRecordRawOutput(9-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (11)
core/util/helpers.go (1)
3-17: LGTM! Clean generic helper for optional SQL parameters.The implementation correctly handles nil-to-NULL mapping and applies transformations when values are present. The documentation and example usage are clear.
core/contractsapi/stream_procedures.go (4)
133-135: LGTM! Correctly migrated to shared utility.The optional date parameters now use
util.TransformOrNil, which centralizes the nil-handling logic previously implemented locally.
213-213: LGTM! Type alias updated to reference shared definition.The alias now points to
types.GetRecordRawOutput, eliminating the local duplicate and centralizing the type definition.
219-222: LGTM! Consistent use of shared utility for optional parameters.All optional date parameters (
From,To,FrozenAt,BaseDate) now useutil.TransformOrNil, maintaining consistency withGetRecord.
301-301: LGTM! Consistent type centralization.Another alias updated to reference the shared
types.GetRecordRawOutput, maintaining consistency across the codebase.core/tnclient/actions_transport.go (6)
31-84: LGTM! GetRecord implementation addresses previous review feedback.The implementation correctly:
- Includes the
BaseDateparameter (line 38) that was previously missing- Handles EventTime parsing errors (lines 69-72) instead of silently ignoring them
- Uses the shared
util.TransformOrNilhelper for optional parameters- Decodes via the centralized
clientType.GetRecordRawOutputtypeThe comment acknowledging missing cache metadata parsing is appropriate for this minimal transport implementation.
89-99: LGTM! Clean delegation to transport layer.Both
ExecuteProcedureandCallProcedurecorrectly delegate to the underlying transport with appropriate unwrapping and error handling.
101-171: LGTM! Stub implementations are clear and appropriately documented.The stub methods consistently return "not implemented" errors with clear guidance. The comments (lines 86-87) and docstrings appropriately set expectations that this is a minimal implementation focused on QuantAMM's needs.
173-182: LGTM! Clean embedding structure for primitive actions.Embedding
TransportActionallowsTransportPrimitiveActionto inherit the base functionality while adding primitive-specific methods. Documentation clearly indicates the minimal implementation scope.
186-213: LGTM! Correct batched insert with proper decimal conversion.The implementation correctly:
- Converts float64 values to Kwil decimals with 36 precision and 18 scale
- Batches all records into single arrays for efficient execution
- Uses
strconv.FormatFloatwith -1 precision to preserve full precision- Forwards transaction options properly
215-224: LGTM! Clean delegation pattern and consistent stub.
InsertRecordefficiently delegates toInsertRecords, avoiding code duplication. TheCheckValidPrimitiveStreamstub is consistent with other unimplemented methods.
1 participant
resolves: https://github.com/truflation/website/issues/3054
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.