Feat/noble ibc testnet - #1307
Conversation
This commit integrates the Noble testnet (grand-1) with Sonr's x/dex module, enabling native USDC trading and liquidity operations across IBC-enabled chains. Changes: - Add default params configuration with Noble testnet in allowed connections - Create Noble-specific helper types and functions for USDC operations - Implement USDC conversion utilities (base units <-> USDC decimals) - Add NobleSwapParams and NobleLiquidityParams for structured operations - Update genesis state to use default params with validation - Add comprehensive Noble integration documentation - Create unit tests for params and Noble helpers Noble Configuration: - Chain ID: noble-grand-1 (testnet) - USDC Denom: uusdc (6 decimals) - RPC: https://noble-testnet-rpc.polkachu.com:443 - gRPC: noble-testnet-grpc.polkachu.com:21590 Module Parameters: - Max accounts per DID: 5 - Default ICA timeout: 600 seconds - Min swap amount: 1000 base units - Max daily volume: 1T base units - Rate limits: 10 ops/block, 100 ops/DID/day - Fees: 0.3% swap, 0.2% liquidity, 0.1% orders This integration enables Sonr users to: - Register ICA accounts on Noble testnet - Execute cross-chain swaps with USDC - Provide/remove liquidity in USDC pairs - Trade with slippage protection - Route multi-hop swaps through USDC 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
## feat(dex): add support for noble usdc swaps
WalkthroughThis pull request introduces E2E testing infrastructure for USDC swaps with DID-based Interchain Accounts, implements cross-chain swap execution via the DEX module, and adds Noble chain integration utilities with comprehensive validation and parameter management. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as E2E Test Client
participant Chain as Sonr Chain
participant DID as DID Module
participant DEX as DEX Module
participant ICA as ICA Module
participant Noble as Noble Chain
Client->>Chain: 1. Create DID Document
Chain->>DID: Register DID
DID-->>Client: DID Document Created
Client->>Chain: 2. Register DEX Account (Noble connection)
Chain->>DEX: Store InterchainDEXAccount
DEX-->>Client: DEX Account Registered
Client->>Chain: 3. Submit ExecuteSwap TX
Chain->>DEX: Validate UCAN + Parameters
DEX->>DEX: Build Noble Swap Message
DEX->>ICA: Submit ICA TX (swap via Noble)
ICA->>Noble: Execute Swap (USDC ↔ SNR)
Noble-->>ICA: Swap Executed (event emitted)
ICA-->>DEX: ICA Response received
DEX->>DEX: Emit SwapExecuted Event
DEX-->>Chain: Return Sequence + Metadata
Chain-->>Client: TX Confirmed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–80 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Command failed Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 40
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/e2e/client/chain.go (2)
139-176: Improve HTTP error reporting: include response body for non-200 replies.Currently returns only status code, losing diagnostic context.
+import ( + "io" +) @@ - if resp.StatusCode != http.StatusOK { + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) if attempt == maxRetries-1 { - return fmt.Errorf("request failed with status %d", resp.StatusCode) + return fmt.Errorf("request failed with status %d: %s", resp.StatusCode, strings.TrimSpace(string(body))) } time.Sleep(retryDelay) continue }
46-55: Minor: avoid naming locals ‘url’ to reduce confusion with package net/url.Rename locals like url := fmt.Sprintf(...) to endpoint for readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (19)
Makefile(3 hunks)test/e2e/client/chain.go(1 hunks)test/e2e/go.mod(3 hunks)test/e2e/usdc-swap-did/Makefile(1 hunks)test/e2e/usdc-swap-did/README.md(1 hunks)test/e2e/usdc-swap-did/config.example.yaml(1 hunks)test/e2e/usdc-swap-did/helpers.go(1 hunks)test/e2e/usdc-swap-did/setup.sh(1 hunks)test/e2e/usdc-swap-did/usdc_swap_test.go(1 hunks)x/dex/NOBLE_INTEGRATION.md(1 hunks)x/dex/client/cli/tx.go(4 hunks)x/dex/keeper/msg_server.go(5 hunks)x/dex/keeper/swap.go(1 hunks)x/dex/types/errors.go(1 hunks)x/dex/types/genesis.go(2 hunks)x/dex/types/noble.go(1 hunks)x/dex/types/noble_test.go(1 hunks)x/dex/types/params.go(1 hunks)x/dex/types/params_test.go(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
test/e2e/usdc-swap-did/Makefile
[warning] 73-73: Target body for "setup" exceeds allowed length of 5 (8).
(maxbodylength)
[warning] 125-125: Target body for "help" exceeds allowed length of 5 (20).
(maxbodylength)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 4-4: Target "all" should be declared PHONY.
(phonydeclared)
🪛 markdownlint-cli2 (0.18.1)
test/e2e/usdc-swap-did/README.md
46-46: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
54-54: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
62-62: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
75-75: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
85-85: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
91-91: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
95-95: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
105-105: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
113-113: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
121-121: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
189-189: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
190-190: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
198-198: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
199-199: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
210-210: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
211-211: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
279-279: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
280-280: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
280-280: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
282-282: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
285-285: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
286-286: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
286-286: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
288-288: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
291-291: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
292-292: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
292-292: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
294-294: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
297-297: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
298-298: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
298-298: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
300-300: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
368-368: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
397-397: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
x/dex/NOBLE_INTEGRATION.md
300-300: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
306-306: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
311-311: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
317-317: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
323-323: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
329-329: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
337-337: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
338-338: Bare URL used
(MD034, no-bare-urls)
339-339: Bare URL used
(MD034, no-bare-urls)
340-340: Bare URL used
(MD034, no-bare-urls)
342-342: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
343-343: Bare URL used
(MD034, no-bare-urls)
344-344: Bare URL used
(MD034, no-bare-urls)
346-346: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
347-347: Bare URL used
(MD034, no-bare-urls)
348-348: Bare URL used
(MD034, no-bare-urls)
🪛 Shellcheck (0.11.0)
test/e2e/usdc-swap-did/setup.sh
[info] 92-92: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 107-107: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 120-120: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 130-130: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 140-140: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 144-144: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 157-157: Double quote to prevent globbing and word splitting.
(SC2086)
🔇 Additional comments (20)
test/e2e/client/chain.go (2)
353-357: Wrapper LGTM.Public DoRequest cleanly exposes the private helper for tests.
359-383: TxResponse struct LGTM.Fields match common Cosmos REST response shapes; suitable for E2E parsing.
x/dex/types/params.go (1)
112-133: Fee validation LGTM.Upper-bound checks are sufficient given unsigned types.
x/dex/types/genesis.go (1)
8-8: LGTM! Parameter initialization and validation properly integrated.The changes correctly initialize
ParamswithDefaultParams()in both constructors and add validation in theValidate()method, following standard Cosmos SDK patterns.Also applies to: 16-16, 27-30
test/e2e/go.mod (1)
22-22: LGTM! Dependency management properly updated.The replace directive for
protoc-gen-validatecorrectly points to the official envoyproxy fork at v1.1.0, which is the maintained successor to the lyft version.Also applies to: 425-425
x/dex/client/cli/tx.go (3)
78-89: Excellent documentation improvements.The comprehensive
Longdescription with concrete examples significantly improves usability. The examples clearly demonstrate both Noble testnet and Osmosis swap scenarios.
111-134: Verify timeout semantics for message execution.Line 134 sets
Timeoutas an absolute timestamp (time.Now().Add(timeoutDuration)), which begins counting from when the CLI command is executed. If the transaction sits in the mempool or takes time to be included in a block, the effective timeout window may be shorter than intended.Consider whether the timeout should instead be calculated at message execution time (in the message server) or if the current behavior is acceptable for your use case.
145-147: LGTM! Flag definitions are clear and well-documented.The optional flags provide good flexibility for advanced use cases while maintaining sensible defaults.
Makefile (1)
256-267: LGTM! E2E test targets properly integrated.The new targets are well-structured with appropriate logging, and the warning about manual setup requirements is helpful for users. The help documentation clearly explains each target's purpose.
Also applies to: 322-322, 418-422
x/dex/types/params_test.go (1)
1-311: Excellent test coverage for parameter validation.The test suite is comprehensive and well-structured:
- Table-driven approach with clear test case names
- Coverage of both valid and invalid scenarios
- Proper error message validation
- Tests align well with the validation logic in the implementation
x/dex/types/noble_test.go (1)
1-533: Comprehensive test coverage for Noble integration utilities.The test suite thoroughly validates:
- Chain configuration and identification
- Connection validation
- USDC conversion and formatting utilities
- Swap and liquidity parameter validation
- Trading pair utilities
The tests are well-structured with good coverage of both valid and invalid scenarios.
x/dex/keeper/swap.go (3)
175-210: LGTM! Simple but effective routing strategy.The routing logic appropriately uses USDC as an intermediary for indirect swaps. This is a common pattern in DEX implementations and provides a good foundation for future optimization with more sophisticated routing algorithms.
212-227: Placeholder estimation using simple fee model.The current 1% fee model is a reasonable placeholder for testing, but production use will require querying actual exchange rates and pool pricing as noted in the comments (lines 218-221).
Consider integrating with a price oracle or on-chain DEX pool queries before production use to ensure accurate swap estimations.
229-240: LGTM! Slippage calculation is correct.The method properly handles the zero case and correctly calculates slippage as a percentage. The formula
(expectedOutput - minOutput) / expectedOutput * 100is the standard approach for slippage calculation.x/dex/types/noble.go (2)
56-60: LGTM: chain ID recognition helper is clear and minimal.
121-149: Validation methods are sound.Good checks on positivity, non-empty fields, and bech32 address.
Also applies to: 167-189
test/e2e/usdc-swap-did/usdc_swap_test.go (1)
56-61: Hardcoded NobleConnectionID should be parameterized from config to avoid test brittleness.The constant "connection-noble" can diverge from actual IBC connection IDs (e.g., "connection-0" in other tests), causing transaction failures. However, the secondary concern about MsgExecuteSwap's Timeout field is not applicable—it correctly uses
time.Timetype, notuint64.Recommendation: Read NobleConnectionID and NobleUSDCDenom from test config/env rather than constants across all usages (lines 162–169, 214–223, 253–262, 332–341, 436–444, 485–493).
test/e2e/usdc-swap-did/config.example.yaml (1)
28-33: ---Endpoints verified as current — no action required.
The REST endpoint (https://noble-testnet-api.polkachu.com), RPC endpoint (https://noble-testnet-rpc.polkachu.com:443), and gRPC endpoint (noble-testnet-grpc.polkachu.com:21590) are confirmed current for October 2025. The chain ID "grand-1" aligns with official documentation. The example configuration is accurate and ready for publication.
test/e2e/usdc-swap-did/helpers.go (2)
17-28: LGTM: CreateTestDID helper behavior is clear and safe for E2E.Gracefully returns existing DID or actionable CLI instructions. Good guardrail for tests.
85-100: LGTM: QueryAllDEXAccounts helper is straightforward.Simple pass-through with contextual error. No issues.
| // SignAndBroadcastTx signs and broadcasts a transaction message | ||
| // Note: This is a simplified implementation for E2E tests | ||
| // In production, use proper keyring and transaction signing | ||
| func (c *StarshipClient) SignAndBroadcastTx(ctx context.Context, from string, msgs ...sdk.Msg) (*TxResponse, error) { | ||
| // For E2E tests, we'll use the REST API to broadcast transactions | ||
| // In a real implementation, you would: | ||
| // 1. Sign the transaction with the account's private key | ||
| // 2. Encode the transaction | ||
| // 3. Broadcast via REST or gRPC | ||
|
|
||
| // This is a placeholder that needs proper implementation based on your keyring setup | ||
| // For now, return an error indicating this needs to be implemented | ||
| return nil, fmt.Errorf("SignAndBroadcastTx not yet implemented - use CLI or proper SDK client for transaction signing") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Stubbed SignAndBroadcastTx will fail callers; either implement minimal broadcast or guard usage.
- If unused in this PR, keep but clearly mark not for tests and add TODO.
- If tests will call it, implement basic broadcast (signed tx bytes) or remove the method to avoid dead API.
Check for call sites:
If needed, here’s a minimal improvement that preserves API but clarifies status:
func (c *StarshipClient) SignAndBroadcastTx(ctx context.Context, from string, msgs ...sdk.Msg) (*TxResponse, error) {
- // This is a placeholder that needs proper implementation based on your keyring setup
- // For now, return an error indicating this needs to be implemented
- return nil, fmt.Errorf("SignAndBroadcastTx not yet implemented - use CLI or proper SDK client for transaction signing")
+ // Not implemented: signing requires keyring; intentionally unsupported in E2E client.
+ // Use CLI in tests or provide pre-signed tx bytes via a dedicated helper.
+ return nil, fmt.Errorf("not implemented: use CLI or SDK for signing; E2E client does not sign transactions")
}🏁 Script executed:
#!/bin/bash
rg -nP '\bSignAndBroadcastTx\s*\(' -C2Length of output: 336
The search found a different SignAndBroadcastTx method (on *SonrContext, not *StarshipClient). Let me search more specifically for actual calls to the method under review.
🏁 Script executed:
#!/bin/bash
# Search for calls to StarshipClient's SignAndBroadcastTx or any .SignAndBroadcastTx calls in test files
rg -nP '\.SignAndBroadcastTx\s*\(' test/ -C2Length of output: 45
🏁 Script executed:
#!/bin/bash
# Also check the specific e2e test directory for any usage
rg -nP 'SignAndBroadcastTx' test/e2e/ -C2Length of output: 49
The test directory structure doesn't match. Let me search more broadly across the repository:
🏁 Script executed:
#!/bin/bash
# Find all files named chain.go
fd -name chain.goLength of output: 230
🏁 Script executed:
#!/bin/bash
# Search for any .SignAndBroadcastTx calls across entire repo
rg -nP '\.SignAndBroadcastTx\s*\(' -C2Length of output: 0
Let me correct the search and verify the file exists:
🏁 Script executed:
#!/bin/bash
# Find chain.go without restrictive flags
find . -name chain.go 2>/dev/nullLength of output: 26
🏁 Script executed:
#!/bin/bash
# Search for SignAndBroadcastTx definition (any occurrence)
rg 'SignAndBroadcastTx' -C1Length of output: 279
Let me examine the actual file under review to understand the current state:
🏁 Script executed:
#!/bin/bash
# Check the content around lines 385-398 in chain.go
sed -n '380,400p' ./test/e2e/client/chain.goLength of output: 920
🏁 Script executed:
#!/bin/bash
# Also search for StarshipClient definition/struct
rg 'type StarshipClient' -C3Length of output: 0
Let me check if the method is called within test files or if there are any usages in chain.go itself:
🏁 Script executed:
#!/bin/bash
# Look for any test functions in chain.go that might call SignAndBroadcastTx
grep -n 'SignAndBroadcastTx\|Test\|func' ./test/e2e/client/chain.go | head -30Length of output: 1852
🏁 Script executed:
#!/bin/bash
# Search for all calls in test/e2e/ directory without false path errors
find ./test/e2e -name '*.go' -type f -exec grep -l 'SignAndBroadcastTx' {} \;Length of output: 102
The method IS being used! Let me check the actual call sites:
🏁 Script executed:
#!/bin/bash
# Check how SignAndBroadcastTx is called in dex_test.go
grep -n 'SignAndBroadcastTx' ./test/e2e/tests/dex/dex_test.go -B2 -A2Length of output: 3187
🏁 Script executed:
#!/bin/bash
# Check how SignAndBroadcastTx is called in usdc_swap_test.go
grep -n 'SignAndBroadcastTx' ./test/e2e/usdc-swap-did/usdc_swap_test.go -B2 -A2Length of output: 2006
Implement SignAndBroadcastTx or tests will fail; method is called by 15+ test cases expecting success.
The method is actively used in test/e2e/tests/dex/dex_test.go (15+ call sites) and test/e2e/usdc-swap-did/usdc_swap_test.go (10+ call sites). All callers expect err == nil and txResp.Code == 0 (success), so the current error-returning stub will fail the entire test suite.
The usdc_swap_test.go file even documents this as a known limitation (lines 9-12): "The SignAndBroadcastTx method in StarshipClient is a placeholder that requires proper keyring integration to function."
Options:
- Implement proper transaction signing/broadcasting with keyring support
- Remove the method and refactor tests to use an alternative signing approach
- If intentionally stubbed for partial testing, wrap all callers with try-catch or conditional guards and document why
Suggested improved comment:
// Not implemented: signing requires keyring integration.
// Tests currently call this method expecting success; either implement proper signing
// or refactor test setup to bypass transaction signing (e.g., use pre-signed tx bytes).
return nil, fmt.Errorf("not implemented: use CLI or SDK for signing; E2E client does not sign transactions")🤖 Prompt for AI Agents
test/e2e/client/chain.go around lines 385-398: the SignAndBroadcastTx method is
a stub that always returns an error which breaks 15+ E2E tests; replace the stub
with a real implementation that (1) resolves the "from" account key (keyring or
in-memory private key used for tests), (2) constructs a TxFactory/TxBuilder with
the correct chainID, gas, fees and sign mode, (3) sets the provided msgs onto
the builder, (4) signs the tx using the account's key (via keyring.Sign or
direct privkey.Sign) and encodes it, and (5) broadcasts the signed tx via the
node's REST/gRPC BroadcastTx endpoint and returns the parsed TxResponse; if
adding keyring support is infeasible now, instead update tests to use pre-signed
tx bytes or a CLI-based broadcast and document the limitation clearly so callers
no longer expect SignAndBroadcastTx to succeed.
| # Connection from Sonr to Noble | ||
| # Update this with your actual connection ID | ||
| noble_connection_id: "connection-0" | ||
|
|
||
| # IBC transfer channel | ||
| # Update with actual channel after connection | ||
| transfer_channel_id: "channel-0" | ||
|
|
There was a problem hiding this comment.
Connection/channel ID defaults diverge from tests; align or make explicit.
This example uses “connection-0”/“channel-0” while tests default to “connection-noble”. Mismatch will cause registration/swaps to fail.
- Pick a single canonical default (e.g., “connection-noble”), or
- Make tests read from config.yaml instead of hardcoded constants.
Want a patch to wire tests to this config?
Also applies to: 57-58
| # DID Configuration | ||
| did: | ||
| # DID prefix | ||
| prefix: "did:snr:test" | ||
|
|
||
| # Generate unique DIDs per test | ||
| unique_per_test: true | ||
|
|
||
| # DID document configuration | ||
| document: | ||
| verification_methods: | ||
| - type: "EcdsaSecp256k1VerificationKey2019" | ||
| purpose: ["authentication", "assertionMethod"] | ||
|
|
||
| services: | ||
| - type: "DEXSwapService" | ||
| endpoint: "https://dex.sonr.io" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
DID document example: add key material guidance or mark placeholders.
Downstream creation helpers may need key types/IDs. Consider adding comments noting keys/services here are illustrative.
| url := fmt.Sprintf("%s/sonr/did/v1/did/%s", cfg.BaseURL, didID) | ||
|
|
There was a problem hiding this comment.
Harden URL construction: escape path segments to prevent malformed requests/path injection.
Use net/url PathEscape for didID/connectionID. Also import the package (aliased) to avoid name shadowing.
Apply these diffs:
@@
import (
"context"
"fmt"
"time"
"github.com/sonr-io/sonr/test/e2e/utils"
dextypes "github.com/sonr-io/sonr/x/dex/types"
didtypes "github.com/sonr-io/sonr/x/did/types"
+ urlpkg "net/url"
)
@@
- url := fmt.Sprintf("%s/sonr/did/v1/did/%s", cfg.BaseURL, didID)
+ url := fmt.Sprintf("%s/sonr/did/v1/did/%s", cfg.BaseURL, urlpkg.PathEscape(didID))
@@
- url := fmt.Sprintf("%s/sonr/dex/v1/account/%s/%s", cfg.BaseURL, didID, connectionID)
+ url := fmt.Sprintf("%s/sonr/dex/v1/account/%s/%s", cfg.BaseURL, urlpkg.PathEscape(didID), urlpkg.PathEscape(connectionID))
@@
- url := fmt.Sprintf("%s/sonr/dex/v1/history/%s", cfg.BaseURL, didID)
+ url := fmt.Sprintf("%s/sonr/dex/v1/history/%s", cfg.BaseURL, urlpkg.PathEscape(didID))
@@
- url := fmt.Sprintf("%s/ibc/core/connection/v1/connections/%s", cfg.BaseURL, connectionID)
+ url := fmt.Sprintf("%s/ibc/core/connection/v1/connections/%s", cfg.BaseURL, urlpkg.PathEscape(connectionID))Also applies to: 51-52, 69-70, 243-244, 3-11
| func WaitForDEXAccountActivation(ctx context.Context, cfg *utils.TestConfig, didID, connectionID string, timeout time.Duration) (bool, error) { | ||
| deadline := time.Now().Add(timeout) | ||
|
|
||
| for time.Now().Before(deadline) { | ||
| account, err := QueryDEXAccount(ctx, cfg, didID, connectionID) | ||
| if err != nil { | ||
| // Account not found yet, continue waiting | ||
| time.Sleep(cfg.BlockTime) | ||
| continue | ||
| } | ||
|
|
||
| if account.Status == dextypes.ACCOUNT_STATUS_ACTIVE { | ||
| return true, nil | ||
| } | ||
|
|
||
| // Check for failed status | ||
| if account.Status == dextypes.ACCOUNT_STATUS_FAILED { | ||
| return false, fmt.Errorf("DEX account activation failed") | ||
| } | ||
|
|
||
| time.Sleep(cfg.BlockTime) | ||
| } | ||
|
|
||
| return false, fmt.Errorf("timeout waiting for account activation") | ||
| } |
There was a problem hiding this comment.
Fix: possible nil deref + ignore of context cancellation in activation wait loop.
If the server returns 200 with a null account, dereferencing Status will panic. The loop also ignores ctx.Done and may spin if BlockTime==0.
Apply this diff:
func WaitForDEXAccountActivation(ctx context.Context, cfg *utils.TestConfig, didID, connectionID string, timeout time.Duration) (bool, error) {
deadline := time.Now().Add(timeout)
+ sleep := cfg.BlockTime
+ if sleep <= 0 {
+ sleep = 2 * time.Second
+ }
for time.Now().Before(deadline) {
+ if err := ctx.Err(); err != nil {
+ return false, err
+ }
account, err := QueryDEXAccount(ctx, cfg, didID, connectionID)
if err != nil {
- // Account not found yet, continue waiting
- time.Sleep(cfg.BlockTime)
+ if ctx.Err() != nil {
+ return false, ctx.Err()
+ }
+ time.Sleep(sleep)
continue
}
+ if account == nil {
+ time.Sleep(sleep)
+ continue
+ }
if account.Status == dextypes.ACCOUNT_STATUS_ACTIVE {
return true, nil
}
// Check for failed status
if account.Status == dextypes.ACCOUNT_STATUS_FAILED {
return false, fmt.Errorf("DEX account activation failed")
}
- time.Sleep(cfg.BlockTime)
+ time.Sleep(sleep)
}
return false, fmt.Errorf("timeout waiting for account activation")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func WaitForDEXAccountActivation(ctx context.Context, cfg *utils.TestConfig, didID, connectionID string, timeout time.Duration) (bool, error) { | |
| deadline := time.Now().Add(timeout) | |
| for time.Now().Before(deadline) { | |
| account, err := QueryDEXAccount(ctx, cfg, didID, connectionID) | |
| if err != nil { | |
| // Account not found yet, continue waiting | |
| time.Sleep(cfg.BlockTime) | |
| continue | |
| } | |
| if account.Status == dextypes.ACCOUNT_STATUS_ACTIVE { | |
| return true, nil | |
| } | |
| // Check for failed status | |
| if account.Status == dextypes.ACCOUNT_STATUS_FAILED { | |
| return false, fmt.Errorf("DEX account activation failed") | |
| } | |
| time.Sleep(cfg.BlockTime) | |
| } | |
| return false, fmt.Errorf("timeout waiting for account activation") | |
| } | |
| func WaitForDEXAccountActivation(ctx context.Context, cfg *utils.TestConfig, didID, connectionID string, timeout time.Duration) (bool, error) { | |
| deadline := time.Now().Add(timeout) | |
| sleep := cfg.BlockTime | |
| if sleep <= 0 { | |
| sleep = 2 * time.Second | |
| } | |
| for time.Now().Before(deadline) { | |
| if err := ctx.Err(); err != nil { | |
| return false, err | |
| } | |
| account, err := QueryDEXAccount(ctx, cfg, didID, connectionID) | |
| if err != nil { | |
| if ctx.Err() != nil { | |
| return false, ctx.Err() | |
| } | |
| time.Sleep(sleep) | |
| continue | |
| } | |
| if account == nil { | |
| time.Sleep(sleep) | |
| continue | |
| } | |
| if account.Status == dextypes.ACCOUNT_STATUS_ACTIVE { | |
| return true, nil | |
| } | |
| // Check for failed status | |
| if account.Status == dextypes.ACCOUNT_STATUS_FAILED { | |
| return false, fmt.Errorf("DEX account activation failed") | |
| } | |
| time.Sleep(sleep) | |
| } | |
| return false, fmt.Errorf("timeout waiting for account activation") | |
| } |
🤖 Prompt for AI Agents
In test/e2e/usdc-swap-did/helpers.go around lines 104 to 128, the wait loop can
nil-deref account when the server returns 200 with a null account and it ignores
context cancellation and may spin if cfg.BlockTime==0; update the loop to first
check ctx.Done and return ctx.Err() when cancelled, after QueryDEXAccount handle
nil account by treating it as not-ready (continue) instead of accessing
account.Status, and replace time.Sleep(cfg.BlockTime) with a select that waits
on either ctx.Done or time.After(max(cfg.BlockTime, minimalNonZeroDuration)) to
prevent tight spinning when BlockTime is zero. Ensure the failed/active checks
only run when account != nil and propagate errors from QueryDEXAccount only when
they are non-404/non-transient.
| // ConvertToUSDC converts an amount from base units to USDC representation | ||
| // For example: 1000000 base units = 1.000000 USDC | ||
| func ConvertToUSDC(amount math.Int) math.LegacyDec { | ||
| // Convert to decimal and divide by 10^6 | ||
| return math.LegacyNewDecFromInt(amount).QuoInt64(1000000) | ||
| } | ||
|
|
||
| // ConvertFromUSDC converts USDC amount to base units | ||
| // For example: 1.5 USDC = 1500000 base units | ||
| func ConvertFromUSDC(usdcAmount math.LegacyDec) math.Int { | ||
| // Multiply by 10^6 and truncate to integer | ||
| return usdcAmount.MulInt64(1000000).TruncateInt() | ||
| } | ||
|
|
||
| // FormatUSDCAmount formats a USDC amount for display | ||
| // For example: 1500000 -> "1.500000 USDC" | ||
| func FormatUSDCAmount(amount math.Int) string { | ||
| usdcDec := ConvertToUSDC(amount) | ||
| return fmt.Sprintf("%s USDC", usdcDec.String()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
USDC conversion helpers: small polish.
Consider using a Dec constant for 10^6 to avoid repeating the literal and ease future changes.
Example:
var usdcScale = math.LegacyNewDec(1_000_000)
return math.LegacyNewDecFromInt(amount).Quo(usdcScale)🤖 Prompt for AI Agents
In x/dex/types/noble.go around lines 71 to 90, extract the hard-coded 10^6
literal into a package-level math.LegacyDec constant (e.g., usdcScale :=
math.LegacyNewDec(1000000) declared at top-level) and update the three helpers
to use that constant: use Quo(usdcScale) in ConvertToUSDC, use
Mul(usdcScale).TruncateInt() in ConvertFromUSDC, and keep FormatUSDCAmount
calling ConvertToUSDC (no literal multiplication or division there). Ensure the
constant is named clearly (usdcScale) and located near other constants so future
scale changes are one place.
| // GetNobleUSDCPairs returns common trading pairs that include Noble USDC | ||
| // This is useful for routing swaps through USDC as an intermediary | ||
| func GetNobleUSDCPairs() []TradingPair { | ||
| return []TradingPair{ | ||
| {Base: "uatom", Quote: NobleUSDCDenom, Description: "ATOM/USDC"}, | ||
| {Base: "uosmo", Quote: NobleUSDCDenom, Description: "OSMO/USDC"}, | ||
| {Base: "uakt", Quote: NobleUSDCDenom, Description: "AKT/USDC"}, | ||
| {Base: "ujuno", Quote: NobleUSDCDenom, Description: "JUNO/USDC"}, | ||
| {Base: "ustars", Quote: NobleUSDCDenom, Description: "STARS/USDC"}, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Trading pairs utilities: OK as seed data, but consider making pairs configurable.
Hardcoding pairs is fine for now; exposing via params/config would avoid code releases for updates.
Also applies to: 218-225
🤖 Prompt for AI Agents
x/dex/types/noble.go lines 191-201 (and similarly 218-225): the trading pairs
are hardcoded which forces code changes for updates; refactor to load these
pairs from configuration/params with the current hardcoded list as a default
fallback. Add a function to read pairs from the module's params or a config file
(with validation), return the configured list when present, and otherwise return
the existing literal slice; update any callers to use this getter so future
changes can be done via params without code releases.
| AllowedConnections: []string{ | ||
| // Noble testnet connection - USDC hub for Cosmos | ||
| "noble-grand-1", | ||
| // Osmosis testnet - Primary DEX | ||
| "osmo-test-5", | ||
| // Other testnets can be added here | ||
| }, |
There was a problem hiding this comment.
Clarify that AllowedConnections contains chain IDs (naming is misleading).
Values are chain IDs (“noble-grand-1”, “osmo-test-5”), not IBC connection IDs. This misleads operators.
- AllowedConnections: []string{
- // Noble testnet connection - USDC hub for Cosmos
+ // NOTE: Entries are host chain IDs, not IBC connection IDs.
+ AllowedConnections: []string{
+ // Noble testnet chain ID - USDC hub for Cosmos
"noble-grand-1",
- // Osmosis testnet - Primary DEX
+ // Osmosis testnet chain ID - Primary DEX
"osmo-test-5",
// Other testnets can be added here
},If feasible in a follow-up, rename the field to AllowedChainIDs across proto/types for correctness.
🤖 Prompt for AI Agents
In x/dex/types/params.go around lines 15 to 21, the AllowedConnections slice
actually holds chain IDs (e.g. "noble-grand-1", "osmo-test-5") which is
misleading; update the inline comment to state these are chain IDs, and change
the field name to AllowedChainIDs across proto and Go types (or if not doing
rename now, add a TODO and clearly document the mismatch) — ensure all
references, JSON/protobuf tags, validation, and migration notes are updated to
reflect AllowedChainIDs to avoid confusion for operators.
| MinSwapAmount: "1000", // Minimum 1000 base units | ||
| MaxDailyVolume: "1000000000000", // 1M units daily volume cap | ||
| RateLimits: RateLimitParams{ |
There was a problem hiding this comment.
Fix comment: 1e12, not “1M units”.
- MaxDailyVolume: "1000000000000", // 1M units daily volume cap
+ MaxDailyVolume: "1000000000000", // 1,000,000,000,000 base units daily cap🤖 Prompt for AI Agents
In x/dex/types/params.go around lines 22 to 24, the inline comment for
MaxDailyVolume incorrectly says "1M units" while the value "1000000000000"
equals 1e12; update the comment to reflect the correct magnitude (e.g., "1e12
units daily volume cap" or "1,000,000,000,000 units daily volume cap") so the
comment matches the numeric value.
| // Validate performs basic validation of module parameters. | ||
| func (p Params) Validate() error { | ||
| if p.MaxAccountsPerDid == 0 { | ||
| return fmt.Errorf("max_accounts_per_did must be positive") | ||
| } | ||
|
|
||
| if p.MaxAccountsPerDid > 100 { | ||
| return fmt.Errorf("max_accounts_per_did cannot exceed 100") | ||
| } | ||
|
|
||
| if p.DefaultTimeoutSeconds == 0 { | ||
| return fmt.Errorf("default_timeout_seconds must be positive") | ||
| } | ||
|
|
||
| if p.DefaultTimeoutSeconds > 3600 { | ||
| return fmt.Errorf("default_timeout_seconds cannot exceed 3600 (1 hour)") | ||
| } | ||
|
|
||
| // Validate swap amounts | ||
| if p.MinSwapAmount != "" { | ||
| minSwap, ok := math.NewIntFromString(p.MinSwapAmount) | ||
| if !ok { | ||
| return fmt.Errorf("invalid min_swap_amount: %s", p.MinSwapAmount) | ||
| } | ||
| if minSwap.IsNegative() { | ||
| return fmt.Errorf("min_swap_amount cannot be negative") | ||
| } | ||
| } | ||
|
|
||
| if p.MaxDailyVolume != "" { | ||
| maxVolume, ok := math.NewIntFromString(p.MaxDailyVolume) | ||
| if !ok { | ||
| return fmt.Errorf("invalid max_daily_volume: %s", p.MaxDailyVolume) | ||
| } | ||
| if maxVolume.IsNegative() { | ||
| return fmt.Errorf("max_daily_volume cannot be negative") | ||
| } | ||
| } | ||
|
|
||
| // Validate rate limits | ||
| if err := p.RateLimits.Validate(); err != nil { | ||
| return fmt.Errorf("invalid rate_limits: %w", err) | ||
| } | ||
|
|
||
| // Validate fees | ||
| if err := p.Fees.Validate(); err != nil { | ||
| return fmt.Errorf("invalid fees: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add validation for AllowedConnections to catch misconfigurations early.
Currently not validated; add non-empty and uniqueness checks.
func (p Params) Validate() error {
@@
- // Validate rate limits
+ // Validate allowed connections (chain IDs)
+ if len(p.AllowedConnections) == 0 {
+ return fmt.Errorf("allowed_connections must contain at least one chain ID")
+ }
+ seen := make(map[string]struct{}, len(p.AllowedConnections))
+ for _, id := range p.AllowedConnections {
+ if id == "" {
+ return fmt.Errorf("allowed_connections cannot contain empty entries")
+ }
+ if _, dup := seen[id]; dup {
+ return fmt.Errorf("allowed_connections contains duplicate chain ID: %s", id)
+ }
+ seen[id] = struct{}{}
+ }
+
+ // Validate rate limits
if err := p.RateLimits.Validate(); err != nil {
return fmt.Errorf("invalid rate_limits: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Validate performs basic validation of module parameters. | |
| func (p Params) Validate() error { | |
| if p.MaxAccountsPerDid == 0 { | |
| return fmt.Errorf("max_accounts_per_did must be positive") | |
| } | |
| if p.MaxAccountsPerDid > 100 { | |
| return fmt.Errorf("max_accounts_per_did cannot exceed 100") | |
| } | |
| if p.DefaultTimeoutSeconds == 0 { | |
| return fmt.Errorf("default_timeout_seconds must be positive") | |
| } | |
| if p.DefaultTimeoutSeconds > 3600 { | |
| return fmt.Errorf("default_timeout_seconds cannot exceed 3600 (1 hour)") | |
| } | |
| // Validate swap amounts | |
| if p.MinSwapAmount != "" { | |
| minSwap, ok := math.NewIntFromString(p.MinSwapAmount) | |
| if !ok { | |
| return fmt.Errorf("invalid min_swap_amount: %s", p.MinSwapAmount) | |
| } | |
| if minSwap.IsNegative() { | |
| return fmt.Errorf("min_swap_amount cannot be negative") | |
| } | |
| } | |
| if p.MaxDailyVolume != "" { | |
| maxVolume, ok := math.NewIntFromString(p.MaxDailyVolume) | |
| if !ok { | |
| return fmt.Errorf("invalid max_daily_volume: %s", p.MaxDailyVolume) | |
| } | |
| if maxVolume.IsNegative() { | |
| return fmt.Errorf("max_daily_volume cannot be negative") | |
| } | |
| } | |
| // Validate rate limits | |
| if err := p.RateLimits.Validate(); err != nil { | |
| return fmt.Errorf("invalid rate_limits: %w", err) | |
| } | |
| // Validate fees | |
| if err := p.Fees.Validate(); err != nil { | |
| return fmt.Errorf("invalid fees: %w", err) | |
| } | |
| return nil | |
| } | |
| // Validate performs basic validation of module parameters. | |
| func (p Params) Validate() error { | |
| if p.MaxAccountsPerDid == 0 { | |
| return fmt.Errorf("max_accounts_per_did must be positive") | |
| } | |
| if p.MaxAccountsPerDid > 100 { | |
| return fmt.Errorf("max_accounts_per_did cannot exceed 100") | |
| } | |
| if p.DefaultTimeoutSeconds == 0 { | |
| return fmt.Errorf("default_timeout_seconds must be positive") | |
| } | |
| if p.DefaultTimeoutSeconds > 3600 { | |
| return fmt.Errorf("default_timeout_seconds cannot exceed 3600 (1 hour)") | |
| } | |
| // Validate swap amounts | |
| if p.MinSwapAmount != "" { | |
| minSwap, ok := math.NewIntFromString(p.MinSwapAmount) | |
| if !ok { | |
| return fmt.Errorf("invalid min_swap_amount: %s", p.MinSwapAmount) | |
| } | |
| if minSwap.IsNegative() { | |
| return fmt.Errorf("min_swap_amount cannot be negative") | |
| } | |
| } | |
| if p.MaxDailyVolume != "" { | |
| maxVolume, ok := math.NewIntFromString(p.MaxDailyVolume) | |
| if !ok { | |
| return fmt.Errorf("invalid max_daily_volume: %s", p.MaxDailyVolume) | |
| } | |
| if maxVolume.IsNegative() { | |
| return fmt.Errorf("max_daily_volume cannot be negative") | |
| } | |
| } | |
| // Validate allowed connections (chain IDs) | |
| if len(p.AllowedConnections) == 0 { | |
| return fmt.Errorf("allowed_connections must contain at least one chain ID") | |
| } | |
| seen := make(map[string]struct{}, len(p.AllowedConnections)) | |
| for _, id := range p.AllowedConnections { | |
| if id == "" { | |
| return fmt.Errorf("allowed_connections cannot contain empty entries") | |
| } | |
| if _, dup := seen[id]; dup { | |
| return fmt.Errorf("allowed_connections contains duplicate chain ID: %s", id) | |
| } | |
| seen[id] = struct{}{} | |
| } | |
| // Validate rate limits | |
| if err := p.RateLimits.Validate(); err != nil { | |
| return fmt.Errorf("invalid rate_limits: %w", err) | |
| } | |
| // Validate fees | |
| if err := p.Fees.Validate(); err != nil { | |
| return fmt.Errorf("invalid fees: %w", err) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In x/dex/types/params.go around lines 38 to 88, add validation for the
AllowedConnections field: ensure the slice is non-empty (return an error if nil
or length 0) and check for duplicate or empty entries by iterating the slice,
rejecting any empty string and using a map to detect duplicates; if a duplicate
or empty value is found return a descriptive fmt.Errorf (e.g.
"allowed_connections must be non-empty" or "duplicate allowed_connection: %s")
and include this check before returning nil at the end of Validate().
2 participants
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests