Sync with upstream repo#39
Conversation
* Add Shibarium Bor bridge RPC methods to allowlist and selector * Limit Bor receipts-by-block to Shibarium only * revert EthereumCallSelector.kt --------- Co-authored-by: a10zn8 <a1.cyb3r@gmail.com>
… Chain (#759) Added response_headers field to NativeCallReplyItem proto. Implemented chain-specific header filtering via getResponseHeadersToForward(). BeaconChain forwards: Eth-Consensus-Version, Eth-Consensus-Finalized, Eth-Execution-Optimistic, Eth-Execution-Payload-Blinded/Value, Eth-Consensus-Block-Value.
* add custom headers * add docs
…losed (#770) * Add WebSocket subscription support for Ripple and migrate to ledger_closed - Replace deprecated `ledger` RPC method with `ledger_closed` for polling - Add WebSocket subscription support for Ripple ledger stream - Add RippleCommandParams for native Ripple WS command format (uses "command" instead of "method") - Extend ResponseWSParser to handle Ripple subscription format (type: "ledgerClosed") - Update WsSubscriptionsImpl to extract stream type for Ripple subscriptions - Implement getFromHeader() for parsing ledgerClosed WS events - Add RippleLedgerStreamEvent data class for WS subscription events - Add unit tests for ResponseWSParser and RippleChainSpecific * Remove legacy format
* add more logs to doge upstream * fix other similar cases
* Implement Solana head detection strategies with metrics tracking and testing * Add benchmark tests for Solana BlockSubscribe and SlotSubscribe strategies * Implement metrics logging and strategy configuration for Solana head detection * Add metrics logging and comparison for SLOT_SUBSCRIBE strategy * Refactor SolanaChainSpecificTest: Consolidate tests for slotSubscribe strategy, remove BlockSubscribe tests, and enhance caching logic. Delete SolanaStrategyBenchmark for performance comparison. * add synthetic parentHash, improve tests * Fix linter * Optimize Solana getLatestBlock to use single getEpochInfo call Replace two separate RPC calls (getSlot + getBlockHeight) with a single getEpochInfo call that returns both absoluteSlot and blockHeight * Enhance height estimation logic in SolanaChainSpecific: Implement optimistic height estimation and improve error handling for RPC failures. Update tests to validate new behavior. * Refactor height verification in SolanaChainSpecific: Replace getBlockHeight with getEpochInfo for improved efficiency and accuracy in height estimation. Update logic to handle actual slot and height retrieval.
Bumps [actions/checkout](https://github.com/actions/checkout) from 4 to 6. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4...v6) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [svenstaro/upload-release-action](https://github.com/svenstaro/upload-release-action) from 1.pre.release to 2. - [Release notes](https://github.com/svenstaro/upload-release-action/releases) - [Changelog](https://github.com/svenstaro/upload-release-action/blob/master/CHANGELOG.md) - [Commits](svenstaro/upload-release-action@v1-release...v2) --- updated-dependencies: - dependency-name: svenstaro/upload-release-action dependency-version: '2' dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [gradle/gradle-build-action](https://github.com/gradle/gradle-build-action) from 2 to 3. - [Release notes](https://github.com/gradle/gradle-build-action/releases) - [Commits](gradle/gradle-build-action@v2...v3) --- updated-dependencies: - dependency-name: gradle/gradle-build-action dependency-version: '3' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…839) Bumps the gradle-minor-and-patch group in /foundation with 3 updates: [org.junit.jupiter:junit-jupiter](https://github.com/junit-team/junit-framework), [org.jetbrains.kotlin.jvm](https://github.com/JetBrains/kotlin) and [gradle-wrapper](https://github.com/gradle/gradle). Updates `org.junit.jupiter:junit-jupiter` from 6.0.1 to 6.0.3 - [Release notes](https://github.com/junit-team/junit-framework/releases) - [Commits](junit-team/junit-framework@r6.0.1...r6.0.3) Updates `org.jetbrains.kotlin.jvm` from 2.3.0 to 2.3.21 - [Release notes](https://github.com/JetBrains/kotlin/releases) - [Changelog](https://github.com/JetBrains/kotlin/blob/master/ChangeLog.md) - [Commits](JetBrains/kotlin@v2.3.0...v2.3.21) Updates `gradle-wrapper` from 9.2.0 to 9.5.1 - [Release notes](https://github.com/gradle/gradle/releases) - [Commits](gradle/gradle@v9.2.0...v9.5.1) --- updated-dependencies: - dependency-name: org.junit.jupiter:junit-jupiter dependency-version: 6.0.3 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: gradle-minor-and-patch - dependency-name: org.jetbrains.kotlin.jvm dependency-version: 2.3.21 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: gradle-minor-and-patch - dependency-name: gradle-wrapper dependency-version: 9.5.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: gradle-minor-and-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [org.mockito:mockito-inline](https://github.com/mockito/mockito) from 4.0.0 to 5.2.0. - [Release notes](https://github.com/mockito/mockito/releases) - [Commits](mockito/mockito@v4.0.0...v5.2.0) --- updated-dependencies: - dependency-name: org.mockito:mockito-inline dependency-version: 5.2.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [io.lettuce:lettuce-core](https://github.com/redis/lettuce) from 5.2.2.RELEASE to 7.5.2.RELEASE. - [Release notes](https://github.com/redis/lettuce/releases) - [Changelog](https://github.com/redis/lettuce/blob/7.5.2.RELEASE/RELEASE-NOTES.md) - [Commits](redis/lettuce@5.2.2.RELEASE...7.5.2.RELEASE) --- updated-dependencies: - dependency-name: io.lettuce:lettuce-core dependency-version: 7.5.2.RELEASE dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [nl.jqno.equalsverifier:equalsverifier](https://github.com/jqno/equalsverifier) from 3.10.1 to 4.5. - [Release notes](https://github.com/jqno/equalsverifier/releases) - [Changelog](https://github.com/jqno/equalsverifier/blob/main/CHANGELOG.md) - [Commits](jqno/equalsverifier@equalsverifier-3.10.1...equalsverifier-4.5) --- updated-dependencies: - dependency-name: nl.jqno.equalsverifier:equalsverifier dependency-version: '4.5' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
) Bumps [net.logstash.logback:logstash-logback-encoder](https://github.com/logfellow/logstash-logback-encoder) from 7.2 to 9.0. - [Release notes](https://github.com/logfellow/logstash-logback-encoder/releases) - [Commits](logfellow/logstash-logback-encoder@logstash-logback-encoder-7.2...logstash-logback-encoder-9.0) --- updated-dependencies: - dependency-name: net.logstash.logback:logstash-logback-encoder dependency-version: '9.0' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [org.yaml:snakeyaml](https://bitbucket.org/snakeyaml/snakeyaml) from 1.24 to 2.6. - [Commits](https://bitbucket.org/snakeyaml/snakeyaml/branches/compare/snakeyaml-2.6..snakeyaml-1.24) --- updated-dependencies: - dependency-name: org.yaml:snakeyaml dependency-version: '2.6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [org.yaml:snakeyaml](https://bitbucket.org/snakeyaml/snakeyaml) from 1.24 to 2.6. - [Commits](https://bitbucket.org/snakeyaml/snakeyaml/branches/compare/snakeyaml-2.6..snakeyaml-1.24) --- updated-dependencies: - dependency-name: org.yaml:snakeyaml dependency-version: '2.6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/kotlin/io/emeraldpay/dshackle/upstream/stream/Responses.kt (1)
44-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude
headersinAggregateResponseequality/hash contract.
AggregateResponsenow carriesheaders, butequals/hashCodeignore it. Different responses can be treated as identical, which breaks map/set/cache semantics.Proposed fix
data class AggregateResponse( val response: ByteArray, val code: Int, val headers: Map<String, String> = emptyMap(), ) : Response() { @@ if (!response.contentEquals(other.response)) return false if (code != other.code) return false + if (headers != other.headers) return false return true } override fun hashCode(): Int { var result = response.contentHashCode() result = 31 * result + code + result = 31 * result + headers.hashCode() return result } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/stream/Responses.kt` around lines 44 - 59, AggregateResponse's equals and hashCode currently omit the headers field, causing distinct instances with different headers to be treated as equal; update the equals method in class AggregateResponse to compare headers (e.g., if (headers != other.headers) return false) alongside response and code, and update hashCode to incorporate headers (e.g., result = 31 * result + headers.hashCode()) so headers participate in the equality/hash contract; ensure you reference the existing methods equals and hashCode and the properties response, code, and headers when making the change.
🟠 Major comments (23)
.github/workflows/publish.yaml-11-14 (1)
11-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet
persist-credentials: falseto prevent credential leakage.The checkout action should disable credential persistence to prevent secrets from being exposed in subsequent steps or artifacts.
🛡️ Proposed fix
- uses: actions/checkout@v6 with: ref: ${{ github.ref }} submodules: recursive + persist-credentials: falseApply this change to both checkout steps at lines 11 and 31.
Also applies to: 31-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish.yaml around lines 11 - 14, Update both uses of the checkout action (the actions/checkout@v6 steps) to set persist-credentials: false so credentials are not left available to subsequent steps; locate the two checkout steps (the one currently at lines around the first block and the second around the later block) and add the key persist-credentials: false under their with: sections alongside ref and submodules..github/workflows/publish.yaml-11-11 (1)
11-11:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPin GitHub Actions to commit SHAs for supply chain security.
In a publish workflow, unpinned actions create a supply chain vulnerability where a compromised action could publish malicious artifacts. Pin all actions to specific commit SHAs.
🔒 Example of pinning actions
- - uses: actions/checkout@v6 + - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 # v6.0.0Apply similar pinning to all action references. Use the GitHub Actions version pinning tool or manually look up commit SHAs for each action version.
Also applies to: 16-16, 21-21, 31-31, 36-36, 41-41, 46-46
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish.yaml at line 11, Replace loose version tags like actions/checkout@v6 (and the other action usages at the indicated lines) with immutable commit SHAs: find the corresponding repository tags for each referenced action (e.g., actions/checkout@v6) and update the workflow steps to use the full commit SHA (actions/checkout@<commit-sha>) for every actions/* reference in the publish workflow; you can use the GitHub Actions version pinning tool or the action repo tags page to obtain the correct commit SHAs and apply them consistently to each action invocation..github/workflows/publish.yaml-21-21 (1)
21-21:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReplace archived
gradle-build-actionwith maintained alternative.The
gradle/gradle-build-actionrepository is archived and no longer maintained. The Gradle team now recommends usinggradle/actions/setup-gradleinstead.📦 Proposed fix to use the maintained action
- uses: gradle/gradle-build-action@v3 + uses: gradle/actions/setup-gradle@v4Apply this change to both job steps at lines 21 and 41.
Also applies to: 41-41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/publish.yaml at line 21, Replace the archived action usage "gradle/gradle-build-action@v3" with the maintained Gradle action "gradle/actions/setup-gradle@v3" wherever it appears (the current two job steps that reference that string); update the "uses:" lines to the new value and ensure any step inputs expected by setup-gradle (e.g., gradle-version) are provided if required by the step.src/main/kotlin/io/emeraldpay/dshackle/upstream/lowerbound/GoldLowerBounds.kt-9-9 (1)
9-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInitialize
boundsdefensively to avoid pre-init crashes.
lateinithere makesgetBoundunsafe beforeinit(...)runs, leading toUninitializedPropertyAccessException.Suggested fix
- private lateinit var bounds: Map<Chain, Map<LowerBoundType, ChainsConfig.GoldLowerBound>> + private var bounds: Map<Chain, Map<LowerBoundType, ChainsConfig.GoldLowerBound>> = emptyMap()Also applies to: 21-23
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/lowerbound/GoldLowerBounds.kt` at line 9, Replace the unsafe lateinit property with a defensively initialized empty map so getBound can be called before init; change "private lateinit var bounds: Map<Chain, Map<LowerBoundType, ChainsConfig.GoldLowerBound>>" to "private var bounds: Map<Chain, Map<LowerBoundType, ChainsConfig.GoldLowerBound>> = emptyMap()" (and apply the same pattern to the other properties referenced around lines 21-23), and ensure the existing init(...) method still assigns a populated map into bounds when called.src/main/kotlin/io/emeraldpay/dshackle/upstream/ethereum/WsSubscriptionsImpl.kt-53-56 (1)
53-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle non-ledger Ripple streams when deriving subscription IDs.
extractRippleStreamTypeonly mapsstreams: ["ledger"]. For other Ripple streams, the fallback torequest.idcan cause the message filter to drop all events because incoming IDs are type-based.Suggested fix
private fun extractRippleStreamType(request: ChainRequest): String? { if (request.method == "subscribe") { val params = request.params val streams: List<*>? = when (params) { is RippleCommandParams -> params.params["streams"] as? List<*> is ObjectParams -> params.obj["streams"] as? List<*> else -> null } - if (streams?.contains("ledger") == true) { - return "ledgerClosed" - } + if (streams != null) { + return when { + streams.contains("ledger") -> "ledgerClosed" + streams.contains("transactions") || streams.contains("transactions_proposed") -> "transaction" + streams.contains("validations") -> "validationReceived" + else -> null + } + } } return null }Also applies to: 72-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/ethereum/WsSubscriptionsImpl.kt` around lines 53 - 56, The code currently uses extractRippleStreamType(request) which only maps streams: ["ledger"], causing non-ledger Ripple streams to fall back to request.id and drop events; update the logic in WsSubscriptionsImpl (and/or extend extractRippleStreamType) to parse request.params["streams"] (or equivalent field) and return the actual stream type string when present (e.g., first element of streams array) for use as the subscription id instead of falling back to request.id; apply the same change to the other occurrence around the 72-85 block so all Ripple stream types (not just "ledger") are used as subscription identifiers.src/main/kotlin/io/emeraldpay/dshackle/upstream/aztec/AztecLowerBoundStateDetector.kt-81-90 (1)
81-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not emit synthetic
UNKNOWNbound on first failure.When there is no cached STATE, returning
LowerBoundData(0, UNKNOWN)conflicts with this detector’s contract and can pollute lower-bound processing. Skip emission and retry on next tick.Suggested fix
- // No cache and a malformed first response: best we can do is emit a - // synthetic archive bound. This is the only place STATE=1 is invented; - // see the trade-off in the class KDoc. + // No cache and malformed first response: skip emission and retry later. log.warn( "Aztec upstream {} returned no oldestHistoricBlockNumber and we have no cached STATE: {}", upstream.getId(), reason, ) - return Mono.just(LowerBoundData(0, LowerBoundType.UNKNOWN)) + return Mono.empty()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/aztec/AztecLowerBoundStateDetector.kt` around lines 81 - 90, In the branch inside AztecLowerBoundStateDetector where a malformed first response and no cached STATE currently cause "return Mono.just(LowerBoundData(0, LowerBoundType.UNKNOWN))", stop emitting a synthetic UNKNOWN bound; replace that return with a non-emitting Mono (e.g., Mono.empty()) so the detector skips emission and will retry on the next tick, keep the warn log intact and ensure the method signature still returns a Mono<LowerBoundData>.src/main/kotlin/io/emeraldpay/dshackle/upstream/HttpReader.kt-69-76 (1)
69-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
HttpHeaders.setforcustomHeaders(override) instead ofadd.
HttpReader.ktalready setsContent-Type(and optionallyAuthorization) usingh.add(...); ifcustomHeadersincludes the same keys, usingaddwill emit duplicate header lines upstream and can break auth/parsing.Suggested fix
if (customHeaders.isNotEmpty()) { val headers = Consumer { h: HttpHeaders -> customHeaders.forEach { (key, value) -> - h.add(key, value) + h.set(key, value) } } build = build.headers(headers) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/HttpReader.kt` around lines 69 - 76, The current header-building code in HttpReader.kt uses HttpHeaders.add when applying customHeaders (the Consumer that iterates customHeaders and calls h.add), which can create duplicate header entries for keys like Content-Type or Authorization; change the header application to call HttpHeaders.set (overwrite existing values) instead of add so customHeaders override existing headers before calling build.headers(headers).src/main/kotlin/io/emeraldpay/dshackle/upstream/UpstreamValidator.kt-119-122 (1)
119-122:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard missing versions config before dereference.
Line 119 uses
versionsConfig.get()!!even though the supplier is nullable. If rules are unavailable, this throws and can disrupt periodic validation instead of skipping it.Suggested fix
- val rule = versionsConfig.get()!!.rules.find { rule -> + val rules = versionsConfig.get()?.rules + if (rules == null) { + log.info("No versions config available, skipping validation for upstream ${upstream.getId()}") + return Mono.just(OK) + } + val rule = rules.find { rule -> rule.client == type && (rule.networks.isNullOrEmpty() || rule.networks.any { chain.shortNames.contains(it) }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/UpstreamValidator.kt` around lines 119 - 122, The code dereferences versionsConfig.get() with !! which can throw if the supplier returns null; update the lookup in UpstreamValidator (the block that assigns rule from versionsConfig.get()!!.rules.find) to guard against a null supplier value by first retrieving val cfg = versionsConfig.get() and returning/skipping validation if cfg is null (or using a safe navigation like cfg.rules?.find) so you never call !! on a null and the periodic validator simply continues when rules are unavailable.src/main/kotlin/io/emeraldpay/dshackle/Starter.kt-46-56 (1)
46-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn the executor returned by
ExecutorServiceMetrics.monitor(...)from the scheduler decorator.
ExecutorServiceMetrics.monitor(...)returns an instrumented/proxiedExecutorServiceused for timing/idle metrics. The current decorator discards that return value and always returns the originalexecService, so Reactor may run tasks on the uninstrumented executor and skip the “timed” metrics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/Starter.kt` around lines 46 - 56, The scheduler decorator in Schedulers.addExecutorServiceDecorator is discarding the instrumented executor returned by ExecutorServiceMetrics.monitor and always returning the original execService; change the lambda in Starter.kt so that when you call ExecutorServiceMetrics.monitor(Metrics.globalRegistry, execService, schedulerName) you capture its return value (e.g., monitoredExec) and return that monitored executor instead of execService, keeping the existing schedulerName computation and the conditional check on schedulerName (scanner/Attr.NAME, contains("single") || contains("parallel")) unchanged.src/main/kotlin/io/emeraldpay/dshackle/config/hot/AutoReloadbleConfig.kt-22-28 (1)
22-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit connect/response/connection-request timeouts for hot-reload HTTP fetch (Apache HttpClient 5)
AutoReloadbleConfig.reload()creates the client withHttpClients.createDefault()and executes theHttpGetwithout any timeout-related configuration in this file (lines ~22–28), so hot reload reliability can degrade during long network stalls.
- Configure timeouts explicitly when building the client: use
ConnectionConfigviaPoolingHttpClientConnectionManagerBuilder#setDefaultConnectionConfig(...)for connect timeout, andRequestConfigfor connection-request timeout + response timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/config/hot/AutoReloadbleConfig.kt` around lines 22 - 28, AutoReloadbleConfig.reload currently creates httpClient via HttpClients.createDefault() and executes HttpGet against configUrl with no timeouts; change the httpClient initialization to build an HttpClient with explicit connect, connection-request and response timeouts by using PoolingHttpClientConnectionManagerBuilder#setDefaultConnectionConfig (or a ConnectionConfig) to set the connect timeout and RequestConfig.custom() to set connectionRequestTimeout and responseTimeout, then use that configured client instead of the current httpClient; update references to httpClient and the reload() method to use the new client builder so hot-reload HTTP fetches have explicit timeouts.src/main/kotlin/io/emeraldpay/dshackle/quorum/JsonBroadcastQuorum.kt-18-20 (1)
18-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
isFailed()currently reports failure before any attempt resolves.Returning
result == nullmakes a fresh quorum instantly failed, which can break callers that gate logic onisFailed().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/quorum/JsonBroadcastQuorum.kt` around lines 18 - 20, isFailed() currently returns result == null which flags a newly created JsonBroadcastQuorum as failed; change isFailed() in JsonBroadcastQuorum to only report failure after the quorum has finished resolving (for example return finished && result == null or equivalent), or add a completion flag that is set when the broadcast finishes and use that flag together with result to decide failure; update any place that sets/reads the completion flag so the quorum isn't considered failed before attempts complete.src/main/kotlin/io/emeraldpay/dshackle/upstream/restclient/RestHttpReader.kt-125-127 (1)
125-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle empty non-200 streamed bodies before
.single().This branch lacks the empty-body fallback used in non-stream mode; empty responses can surface as terminal errors instead of a proper
AggregateResponse.Suggested fix
- u.inbound().receive().aggregate().asByteArray().publishOn(httpScheduler) - .map { AggregateResponse(it, t.status().code(), responseHeaders) } + u.inbound().receive().aggregate().asByteArray().publishOn(httpScheduler) + .map { AggregateResponse(it, t.status().code(), responseHeaders) } + .switchIfEmpty { Mono.just(AggregateResponse(ByteArray(0), t.status().code(), responseHeaders)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/restclient/RestHttpReader.kt` around lines 125 - 127, The streaming branch returns a Publisher built from u.inbound().receive().aggregate().asByteArray().publishOn(httpScheduler).map { AggregateResponse(it, t.status().code(), responseHeaders) } but doesn't handle an empty stream before the downstream calls .single(), causing terminal errors on empty non-200 responses; add the same empty-body fallback used in non-stream mode by inserting a defaultIfEmpty (or switchIfEmpty) that yields AggregateResponse(ByteArray(0), t.status().code(), responseHeaders) before .single(), so empty completes produce a proper AggregateResponse instead of an error.src/main/kotlin/io/emeraldpay/dshackle/upstream/lowerbound/ManualLowerBoundService.kt-65-74 (1)
65-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid re-reading head with
!!acrosscanHandle()/getManualLowerBound().
currentHeightcan change between checks; this can still throw at runtime despitecanHandle()passing.Suggested fix
class HeadManualLowerBoundProvider( @@ ) : ManualLowerBoundProvider { override fun getManualLowerBound(): Long { - return upstream.getHead().getCurrentHeight()!! + setting.value + val head = upstream.getHead().getCurrentHeight() ?: return 0 + return head + setting.value } override fun canHandle(): Boolean { + val head = upstream.getHead().getCurrentHeight() return setting.type == ManualLowerBoundType.HEAD && setting.value < 0 && - upstream.getHead().getCurrentHeight() != null && - upstream.getHead().getCurrentHeight()!! + setting.value >= 0 + head != null && + head + setting.value >= 0 } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/lowerbound/ManualLowerBoundService.kt` around lines 65 - 74, The code re-reads upstream.getHead().getCurrentHeight() in both canHandle() and getManualLowerBound(), risking a null or changed value between calls; change the ManualLowerBoundService to capture the head height once and reuse it: in canHandle() read upstream.getHead().getCurrentHeight() into a private nullable property (e.g., lastHeadHeight), perform the checks using lastHeadHeight and set a cached computed lower bound (e.g., lastComputedLowerBound = lastHeadHeight!! + setting.value) when checks pass, and then have getManualLowerBound() return that cached value instead of re-reading upstream; update nullability handling accordingly so you never use !! on a value that can change between calls.foundation/src/main/kotlin/org/drpc/chainsconfig/ChainsConfig.kt-117-119 (1)
117-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
getChainConfigs()may return duplicate chain configs.
chainMap.valuesis keyed by short names, so oneChainConfigwith multiple aliases is returned multiple times.Suggested fix
fun getChainConfigs(): Collection<ChainConfig> { - return chainMap.values + return chains }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@foundation/src/main/kotlin/org/drpc/chainsconfig/ChainsConfig.kt` around lines 117 - 119, getChainConfigs() currently returns chainMap.values which can include the same ChainConfig multiple times when a chain has multiple aliases; change the method to return a de-duplicated collection (e.g., return chainMap.values.toSet() or chainMap.values.distinct()) so each ChainConfig appears only once, keeping the return type as Collection<ChainConfig> and updating the getChainConfigs() implementation accordingly.src/main/kotlin/io/emeraldpay/dshackle/upstream/ethereum/EthereumUpstreamSettingsDetector.kt-221-221 (1)
221-221:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse case-insensitive comparison for HL
fromaddress.
from == hlNativeTxFromis brittle for checksum/mixed-case addresses and can produce false negatives in detection.Suggested fix
- if (from == hlNativeTxFrom) { // system topup transaction + if (from.equals(hlNativeTxFrom, ignoreCase = true)) { // system topup transaction foundHlNativeTx = true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/ethereum/EthereumUpstreamSettingsDetector.kt` at line 221, The equality check comparing the HL native transaction sender is case-sensitive and can miss matches for checksum/mixed-case Ethereum addresses; in the EthereumUpstreamSettingsDetector (where the condition if (from == hlNativeTxFrom) appears) normalize both operands before comparing (e.g., use a case-insensitive compare or toLowerCase()/equalsIgnoreCase) so that 'from' and 'hlNativeTxFrom' are compared without case sensitivity; update the condition accordingly and ensure null-safety if either value can be null.src/main/kotlin/io/emeraldpay/dshackle/rpc/SubscribeChainStatus.kt-43-43 (1)
43-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard empty
sourcesbefore callingFlux.merge(..., sources.size)
sourcesis derived frommultistreamHolder.all()and can become empty after the chain-id filter, makingsources.size == 0. Reactor’sFlux.merge(Publisher<Publisher<T>>, int concurrency)rejects non-positive concurrency (throwsIllegalArgumentException), so this can fail at runtime.Proposed fix
- return Flux.merge(Flux.fromIterable(sources), sources.size) + if (sources.isEmpty()) { + return Flux.empty() + } + return Flux.merge(Flux.fromIterable(sources), sources.size) .doOnError { log.error("Error during sending chain statuses", it) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/rpc/SubscribeChainStatus.kt` at line 43, The current return in SubscribeChainStatus (the Flux.merge(Flux.fromIterable(sources), sources.size) call) can throw when sources.size == 0; guard the empty case by checking the sources collection before merging: if sources.isEmpty return Flux.empty() (or an appropriate empty/mono publisher) otherwise call Flux.merge(Flux.fromIterable(sources), sources.size). Update the code that builds the local variable sources (from multistreamHolder.all()) and the return site so the empty branch avoids calling Flux.merge with zero concurrency.src/main/kotlin/io/emeraldpay/dshackle/upstream/ethereum/EthereumLowerBoundReceiptsDetector.kt-62-62 (1)
62-62:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not hardcode receipts lower bound to
1after a successful gold-bound probe.At Line 62, returning
LowerBoundData(1, LowerBoundType.RECEIPTS)ignores the configured gold bound and can overstate receipts availability for older blocks. Return the validated configured gold-bound height instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/ethereum/EthereumLowerBoundReceiptsDetector.kt` at line 62, The code currently returns a hardcoded LowerBoundData(1, LowerBoundType.RECEIPTS) which ignores the configured gold bound; update the successful gold-bound probe branch to return the validated configured gold bound height instead of 1 (use the existing gold-bound configuration/variable used by this detector) so the Flux emits LowerBoundData(<configuredGoldBoundHeight>, LowerBoundType.RECEIPTS) rather than LowerBoundData(1, ...).src/main/kotlin/io/emeraldpay/dshackle/proxy/ProxyServer.kt-112-112 (1)
112-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winManage the custom Netty event loop lifecycle explicitly.
ProxyServer.start()creates a newMultiThreadIoEventLoopGroupinline and does not retain/dispose the returned server handle:.route(this::setupRoutes) .runOn(MultiThreadIoEventLoopGroup(NioIoHandler.newFactory())) .bindNow()
ProxyStarteralso has no shutdown hook (@PreDestroy/stop) for this server, so the event-loop threads may not be released on shutdown/restarts/tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/proxy/ProxyServer.kt` at line 112, ProxyServer.start() creates a MultiThreadIoEventLoopGroup inline (via MultiThreadIoEventLoopGroup(NioIoHandler.newFactory())) and never retains or shuts it down; update ProxyServer to store the created event loop group (and the bind handle returned by bindNow()) as fields, pass that retained group into .runOn(...) instead of creating it inline, and add a lifecycle stop method (annotate with `@PreDestroy` or wire into ProxyStarter's shutdown) that cleanly shuts down the server handle and calls shutdownGracefully()/close on the stored MultiThreadIoEventLoopGroup to release Netty threads; also ensure ProxyStarter calls this stop hook on application shutdown.src/main/kotlin/io/emeraldpay/dshackle/upstream/calls/DefaultEthereumMethods.kt-736-738 (1)
736-738:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat
eth_sendRawTransactionSyncas a broadcast/special method.The method is allowed for Monad but currently falls back to
AlwaysQuorum; tx-submission methods should follow the same broadcast quorum path as other raw-transaction methods.Proposed fix
@@ private val specialMethods = listOf( "eth_getTransactionCount", "eth_blockNumber", "eth_getBalance", "eth_sendRawTransaction", + "eth_sendRawTransactionSync", ) @@ - "eth_sendRawTransaction", "hmy_sendRawStakingTransaction", "hmy_sendRawTransaction", "klay_sendRawTransaction", "kaia_sendRawTransaction", "Filecoin.MpoolPush" -> BroadcastQuorum() + "eth_sendRawTransaction", "eth_sendRawTransactionSync", "hmy_sendRawStakingTransaction", "hmy_sendRawTransaction", "klay_sendRawTransaction", "kaia_sendRawTransaction", "Filecoin.MpoolPush" -> BroadcastQuorum()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/calls/DefaultEthereumMethods.kt` around lines 736 - 738, The mapping in DefaultEthereumMethods currently lists "eth_sendRawTransactionSync" for Chain.MONAD__MAINNET/TESTNET but it's being treated like AlwaysQuorum; update DefaultEthereumMethods so that "eth_sendRawTransactionSync" is included in the same broadcast/special-methods list used for raw transaction submission (the broadcast quorum path) for Monad chains instead of the AlwaysQuorum fallback. Locate the list/branch that enumerates broadcast methods in the DefaultEthereumMethods class and move or add the "eth_sendRawTransactionSync" entry there so the request routing for eth_sendRawTransactionSync follows the broadcast/special handling like other raw-transaction methods.src/main/kotlin/io/emeraldpay/dshackle/upstream/solana/SolanaChainSpecific.kt-93-117 (1)
93-117:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep block identity consistent with attached notification payload.
On Line 116, the container is built with
actualSlot/actualSlot - 1whilejsonis still the current slot notificationdata. That can emit a block whoseslot/hash/parentHashdo not match its payload bytes.Proposed fix
- makeBlockFromSlot(actualSlot, actualSlot - 1, actualHeight, upstreamId, data) + makeBlockFromSlot(slot, notification.parent, actualHeight, upstreamId, data)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/solana/SolanaChainSpecific.kt` around lines 93 - 117, The code builds a block using actualSlot from getEpochInfo while the payload bytes in data represent the notification slot, causing slot/hash/parentHash mismatches; fix by extracting the notification slot from data (the current slot notification), use that notificationSlot (and notificationSlot - 1 for parent) when calling makeBlockFromSlot and when setting lastCheckedSlots[upstreamId], but keep recording the actualHeight into lastKnownHeights[upstreamId]; update the call in SolanaChainSpecific where makeBlockFromSlot(actualSlot, actualSlot - 1, actualHeight, upstreamId, data) is invoked to use notificationSlot instead of actualSlot and ensure lastCheckedSlots uses notificationSlot as well.src/main/kotlin/io/emeraldpay/dshackle/upstream/ripple/RippleChainSpecific.kt-40-45 (1)
40-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
ledger_closedfields before dereference.Line 43 and Line 44 dereference
jsonNode.get(...)directly; if a malformed/partial response arrives, this throws NPE and breaks head parsing abruptly. Add explicit field validation and return a controlled parse failure.Suggested fix
val jsonNode = Global.objectMapper.readTree(data) - val ledgerHash = jsonNode.get("ledger_hash").asText() - val ledgerIndex = jsonNode.get("ledger_index").asLong() + val ledgerHash = jsonNode.path("ledger_hash").asText(null) + val ledgerIndexNode = jsonNode.path("ledger_index") + if (ledgerHash.isNullOrBlank() || !ledgerIndexNode.isNumber) { + return Mono.error(IllegalArgumentException("Invalid ledger_closed payload")) + } + val ledgerIndex = ledgerIndexNode.asLong()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/ripple/RippleChainSpecific.kt` around lines 40 - 45, In parseBlock, avoid dereferencing jsonNode.get("ledger_hash") and jsonNode.get("ledger_index") directly; first validate that jsonNode.hasNonNull("ledger_hash") and jsonNode.hasNonNull("ledger_index") (and that ledger_index is numeric) before calling asText()/asLong(); if validation fails return a controlled parse failure (e.g., Mono.error(new IllegalArgumentException("Invalid ledger_closed response: missing or invalid fields")) or Mono.empty() per project convention) so head parsing doesn't throw an NPE. Ensure you reference the same symbols: parseBlock, jsonNode, ledger_hash/ledger_index, ledgerHash and ledgerIndex.src/main/kotlin/io/emeraldpay/dshackle/upstream/avm/AvmChainSpecific.kt-123-150 (1)
123-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse configured
chain.chainIdfor genesis network validation.Line 124 reads
chain.chainId, but Line 138 derives expected network fromchain.chainName. This makes the configuredchainIdeffectively unused for matching and can produce false mismatches for names that don’t end with a network token.Suggested fix
- val expectedNetwork = chain.chainName.substringAfterLast(" ").lowercase() + val expectedNetwork = expected.lowercase() if (genesis.network.equals(expectedNetwork, ignoreCase = true)) { return ValidateUpstreamSettingsResult.UPSTREAM_VALID }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/avm/AvmChainSpecific.kt` around lines 123 - 150, The validation currently computes expectedNetwork from chain.chainName but earlier reads chain.chainId into expected, so update validateGenesis to use the configured chain.chainId for comparison: replace the derivation of expectedNetwork from chain.chainName (the substringAfterLast call) with expectedNetwork = expected.lowercase() (or otherwise normalize chain.chainId), then compare genesis.network to that expectedNetwork in the existing equality check and adjust the log message to reference chain.chainId (or expected) instead of chain.chainName; see validateGenesis, expected, expectedNetwork, and genesis.network for locations to change.src/main/kotlin/io/emeraldpay/dshackle/upstream/avm/AvmChainSpecific.kt-65-75 (1)
65-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid
NotImplementedErrorin AVM websocket subscription runtime paths.
AvmChainSpecific.kt(getFromHeader/listenNewHeadsRequest/unsubscribeNewHeadsRequest) throwsNotImplementedError()(a KotlinError). InGenericWsHead.subscribe(),listenNewHeadsRequest()is called insidetry { ... } catch (e: Exception), soNotImplementedErrorwill bypass that catch and may fail harder than intended; there are also no upstreamcatch (Error)handlers to absorb it.Suggested fix
override fun getFromHeader(data: ByteArray, upstreamId: String, api: ChainReader): Mono<BlockContainer> { - throw NotImplementedError() + throw UnsupportedOperationException("AVM does not support websocket subscriptions") } override fun listenNewHeadsRequest(): ChainRequest { - throw NotImplementedError() + throw UnsupportedOperationException("AVM does not support websocket subscriptions") } override fun unsubscribeNewHeadsRequest(subId: Any): ChainRequest { - throw NotImplementedError() + throw UnsupportedOperationException("AVM does not support websocket subscriptions") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/avm/AvmChainSpecific.kt` around lines 65 - 75, The methods getFromHeader, listenNewHeadsRequest, and unsubscribeNewHeadsRequest in AvmChainSpecific currently throw Kotlin's NotImplementedError (an Error), which bypasses Exception catch blocks (e.g., GenericWsHead.subscribe()); change these runtime-path stubs to throw or return Exceptions instead: replace the thrown NotImplementedError() in listenNewHeadsRequest/unsubscribeNewHeadsRequest with an Exception type such as UnsupportedOperationException("...") (so callers' try/catch Exception will catch it), and change getFromHeader to return Mono.error(UnsupportedOperationException("...")) rather than throwing an Error; reference the methods AvmChainSpecific.getFromHeader, AvmChainSpecific.listenNewHeadsRequest, AvmChainSpecific.unsubscribeNewHeadsRequest and the caller GenericWsHead.subscribe when making the change.
🟡 Minor comments (7)
.sdkmanrc-1-1 (1)
1-1:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGradle version mismatch between .sdkmanrc and wrapper.
.sdkmanrcspecifies Gradle9.2.0, but bothgradle/wrapper/gradle-wrapper.propertiesandfoundation/gradle/wrapper/gradle-wrapper.propertiesin this PR use Gradle9.5.1. This inconsistency may cause confusion in local development environments.🔧 Proposed fix
-gradle=9.2.0 +gradle=9.5.1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.sdkmanrc at line 1, Mismatch between Gradle versions: .sdkmanrc lists 9.2.0 while gradle-wrapper.properties files use 9.5.1; update the .sdkmanrc entry to 9.5.1 so the SDKMAN version matches the Gradle wrapper, or alternatively update the wrapper versions to 9.2.0 if you intend to keep that version—ensure the value in .sdkmanrc and the gradle-wrapper.properties files are identical (e.g., change ".sdkmanrc: gradle=9.2.0" to "gradle=9.5.1") and commit the change.buildSrc/src/main/kotlin/chainsconfig.codegen.gradle.kts-144-144 (1)
144-144:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix/clarify the
"app" -> "BlockchainType.ETHEREUM"mappingIn
buildSrc/src/main/kotlin/chainsconfig.codegen.gradle.kts(line 144), the generator maps"app"toBlockchainType.ETHEREUM, but there’s no other usage oftype: app/type="app"in chain YAML/config, andBlockchainTypehas noAPPvalue—so this looks like an unused alias or a typo. Confirm the intended chain type identifier; if it’s incorrect, change/remove the mapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@buildSrc/src/main/kotlin/chainsconfig.codegen.gradle.kts` at line 144, The mapping entry that returns "app" -> "BlockchainType.ETHEREUM" in chainsconfig.codegen.gradle.kts is likely a typo or stale alias because there is no BlockchainType.APP and no uses of type: app in chain YAML; locate the mapping in chainsconfig.codegen.gradle.kts and either remove the "app" key or replace it with the intended chain identifier (or correct BlockchainType enum constant) after confirming the intended value, and if this was meant to introduce a new enum value, add the corresponding constant to the BlockchainType enum instead of mapping "app" to ETHEREUM; update any YAML/docs that referenced type: app to the corrected identifier.src/test/kotlin/io/emeraldpay/dshackle/upstream/ethereum/EthereumLowerBoundReceiptsDetectorTest.kt-33-42 (1)
33-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset global
GoldLowerBoundsstate after this test.Line 33 mutates global singleton state via
GoldLowerBounds.init(...)but the test does not restore it, which can make later tests order-dependent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/kotlin/io/emeraldpay/dshackle/upstream/ethereum/EthereumLowerBoundReceiptsDetectorTest.kt` around lines 33 - 42, Save the pre-test global state, then restore it after the test: before calling GoldLowerBounds.init(...) capture the existing state (e.g. val prev = GoldLowerBounds.getAll() or similar), and ensure the test restores it in a finally block or `@After` method by calling GoldLowerBounds.init(prev) (or the equivalent reset/clear API) so the global singleton is not left mutated for other tests.src/main/kotlin/io/emeraldpay/dshackle/upstream/ethereum/EthereumLowerBoundProofDetector.kt-45-46 (1)
45-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix single-digit regexes in error patterns.
The patterns on Line 45 and Line 46 use
\d, so they only match one-digit block numbers and miss normal multi-digit heights.Proposed fix
- Regex("block #\\d not found"), - Regex("state at block #\\d is pruned"), + Regex("block #\\d+ not found"), + Regex("state at block #\\d+ is pruned"),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/ethereum/EthereumLowerBoundProofDetector.kt` around lines 45 - 46, The regex patterns in EthereumLowerBoundProofDetector that currently use "\\d" only match a single digit; update the two patterns (the entries created via Regex("block #\\d not found") and Regex("state at block #\\d is pruned")) to use "\\d+" so they match multi-digit block heights (i.e., replace "\\d" with "\\d+" in those Regex constructors).src/main/kotlin/io/emeraldpay/dshackle/upstream/lowerbound/ManualLowerBoundService.kt-84-86 (1)
84-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAllow zero for fixed manual lower bounds.
setting.value > 0rejects0, which is a valid lower-bound value.Suggested fix
- return setting.type == ManualLowerBoundType.FIXED && setting.value > 0 + return setting.type == ManualLowerBoundType.FIXED && setting.value >= 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/lowerbound/ManualLowerBoundService.kt` around lines 84 - 86, The canHandle() predicate in ManualLowerBoundService currently rejects zero because it uses setting.value > 0; change the condition in the canHandle() method (in class ManualLowerBoundService) to allow zero by using setting.value >= 0 while keeping the type check (setting.type == ManualLowerBoundType.FIXED) so fixed manual lower bounds of 0 are accepted.src/main/kotlin/io/emeraldpay/dshackle/upstream/Selector.kt-737-760 (1)
737-760:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize uppercase
Vprefixes before semver parsing.Line 738, Line 748, and Line 759 only strip lowercase
v. If labels/config useV1.2.3, this path returns a falseRangeVersionResponse.Proposed fix
- Semver(actualRawVersion.removePrefix("v"), Semver.SemverType.STRICT) + Semver(actualRawVersion.removePrefix("v").removePrefix("V"), Semver.SemverType.STRICT) - val min = Semver(minRawVersion.removePrefix("v"), Semver.SemverType.STRICT) + val min = Semver(minRawVersion.removePrefix("v").removePrefix("V"), Semver.SemverType.STRICT) - val max = Semver(maxRawVersion.removePrefix("v"), Semver.SemverType.STRICT) + val max = Semver(maxRawVersion.removePrefix("v").removePrefix("V"), Semver.SemverType.STRICT)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/Selector.kt` around lines 737 - 760, Normalize leading 'v' or 'V' before parsing by replacing removePrefix("v") with a case-insensitive trim (e.g., use trimStart('v','V')) for actualRawVersion, minRawVersion and maxRawVersion so Semver(...) receives a normalized string; update the three places where Semver(...) is constructed (the actualSemver creation and the min/max Semver constructions inside the greaterOk and lessOk blocks) to use trimStart('v','V') instead of removePrefix("v").src/main/kotlin/io/emeraldpay/dshackle/rpc/NativeCall.kt-784-804 (1)
784-804:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude
errorAsIsinCallErrorequality/hash contract.
errorAsIsis added at Line 750 and populated at Line 768, but it is ignored byequals/hashCode. TwoCallErrorinstances with different raw error payloads will compare equal, which can hide real differences.Proposed fix
override fun equals(other: Any?): Boolean { if (this === other) return true if (other !is CallError) return false if (id != other.id) return false if (message != other.message) return false if (upstreamError != other.upstreamError) return false if (data != other.data) return false if (upstreamSettingsData != other.upstreamSettingsData) return false + if (errorAsIs != null) { + if (other.errorAsIs == null) return false + if (!errorAsIs.contentEquals(other.errorAsIs)) return false + } else if (other.errorAsIs != null) return false return true } override fun hashCode(): Int { var result = id result = 31 * result + message.hashCode() result = 31 * result + (upstreamError?.hashCode() ?: 0) result = 31 * result + (data?.hashCode() ?: 0) result = 31 * result + upstreamSettingsData.hashCode() + result = 31 * result + (errorAsIs?.contentHashCode() ?: 0) return result }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/rpc/NativeCall.kt` around lines 784 - 804, The equals/hashCode implementation for CallError omits the new field errorAsIs, causing instances with different raw error payloads to compare equal; update CallError.equals to compare errorAsIs (e.g., add a check like if (errorAsIs != other.errorAsIs) return false) and update CallError.hashCode to incorporate errorAsIs (e.g., include 31 * result + (errorAsIs?.hashCode() ?: 0)) so the raw error payload participates in equality and hashing alongside id, message, upstreamError, data, and upstreamSettingsData.
🧹 Nitpick comments (7)
.github/workflows/static.yml (1)
30-30: ⚡ Quick winConsider pinning GitHub Actions to commit SHAs.
The action reference
actions/checkout@v6uses a tag rather than a commit SHA. Security best practices recommend pinning actions to immutable commit SHAs to prevent supply chain attacks.🔒 Example of SHA-pinned action
- name: Checkout uses: actions/checkout@<commit-sha> # v6Note: You can find the commit SHA for v6 in the actions/checkout releases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/static.yml at line 30, The workflow uses the tag actions/checkout@v6 which is mutable; replace the tag with the corresponding immutable commit SHA for the v6 release (e.g., actions/checkout@<commit-sha>) to pin the action, by updating the uses line in the static.yml file; locate the correct SHA from the actions/checkout GitHub releases page and substitute it, and apply the same SHA-pinning approach to any other third-party actions referenced in the workflow..github/workflows/test.yaml (1)
40-40: ⚡ Quick winMigrate off archived
gradle/gradle-build-action
gradle/gradle-build-action@v3is archived; switch the workflow to the supported replacement:uses: gradle/actions/setup-gradle@v3(nearuses: gradle/gradle-build-action@v3).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/test.yaml at line 40, The workflow is using the archived action reference "gradle/gradle-build-action@v3"; update the usage to the supported replacement by changing the action reference to "uses: gradle/actions/setup-gradle@v3" in the workflow file (replace the existing uses: gradle/gradle-build-action@v3 entry), and verify the new action's inputs/outputs in the workflow step match setup-gradle's doc so the Gradle setup step still functions correctly.src/main/kotlin/io/emeraldpay/dshackle/upstream/ChainCallError.kt (1)
48-64: 💤 Low valueVerify that excluding
errorAsIsfrom equality is intentional.The custom
equalsandhashCodeimplementations exclude theerrorAsIsfield from equality comparison. This means twoChainCallErrorinstances with identicalcode,message, anddetailsbut differenterrorAsIsvalues will be considered equal.If
errorAsIsis metadata that shouldn't affect equality semantics, this is correct. However, iferrorAsIsrepresents distinct error states, consider including it in equality comparison usingcontentEquals()for the ByteArray.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/ChainCallError.kt` around lines 48 - 64, The equals/hashCode implementations in ChainCallError currently omit the errorAsIs ByteArray, causing instances that differ only by errorAsIs to be treated equal; update equals in ChainCallError to compare errorAsIs using contentEquals(other.errorAsIs) and update hashCode to incorporate Arrays.hashCode(errorAsIs) (or errorAsIs?.contentHashCode()) so the ByteArray is included in equality semantics and hashing alongside code, message and details.src/main/kotlin/io/emeraldpay/dshackle/upstream/Multistream.kt (1)
453-456: ⚡ Quick winAvoid re-registering gauges on every
UPDATEDevent.
UPDATEDis also emitted from status streams, so callingmonitorUpstream(...)here can cause frequent meter remove/register churn. Consider re-registering only when labels/instance identity actually change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/Multistream.kt` around lines 453 - 456, The UPDATED branch currently calls monitorUpstream(event.upstream) on every UpstreamChangeEvent.ChangeType.UPDATED which causes repeated meter unregister/register churn; change the logic in the UPDATED case (near UpstreamChangeEvent.ChangeType.UPDATED, monitorUpstream, onUpstreamsUpdated, updateUpstreams.emitNext) to first compare the upstream identity/labels that affect metrics (e.g., instance id, service name, or labels on the Upstream object) with a cached/previous value and only call monitorUpstream(event.upstream) when those identity/label fields have actually changed; keep emitting updateUpstreams.emitNext and calling onUpstreamsUpdated unchanged.src/main/kotlin/io/emeraldpay/dshackle/upstream/signature/RsaSigner.kt (1)
25-25: ⚡ Quick winSpecify UTF-8 when converting the wrapped string to bytes for RSA signing
RsaSigner.sign()signswrapped.toByteArray(); whilesourceis derived from the upstreamidand constrained byUpstreamsConfigReaderto ASCII ([a-zA-Z][a-zA-Z0-9_-]+[a-zA-Z0-9]), using an explicit charset avoids any future platform-default surprises and keeps the signing payload canonical.🔧 Suggested fix
+import java.nio.charset.StandardCharsets ... - sig.update(wrapped.toByteArray()) + sig.update(wrapped.toByteArray(StandardCharsets.UTF_8))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/signature/RsaSigner.kt` at line 25, The signing call uses wrapped.toByteArray() which relies on the platform default charset; update RsaSigner.sign() to encode the wrapped string explicitly as UTF-8 before feeding it to the signature engine (replace the sig.update(wrapped.toByteArray()) usage with an explicit UTF-8 byte conversion), ensuring the payload is canonical and deterministic across platforms.src/main/kotlin/io/emeraldpay/dshackle/upstream/calls/DefaultBeaconChainMethods.kt (1)
119-126: ⚡ Quick winPrecompile wildcard patterns to avoid per-request regex allocations.
isCallablerebuilds regex objects on every call. This path is request-hot and can be made allocation-free after startup.♻️ Proposed refactor
private val allowedMethods: Set<String> = beaconMethods + builderMethods + configMethods + debugMethods + nodeMethods + validatorMethods + rewardMethods + + private val wildcardPatterns = allowedMethods + .asSequence() + .filter { it.contains("*") } + .map { it.replace("*", "[^/]+").toRegex() } + .toList() @@ override fun isCallable(method: String): Boolean { if (allowedMethods.contains(method)) { return true } - // Check wildcard patterns (e.g., GET#/eth/v1/beacon/headers/* matches GET#/eth/v1/beacon/headers/head) - return allowedMethods.any { pattern -> - pattern.contains("*") && method.matches(pattern.replace("*", "[^/]+").toRegex()) - } + return wildcardPatterns.any { it.matches(method) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/calls/DefaultBeaconChainMethods.kt` around lines 119 - 126, The isCallable method currently rebuilds regexes per call causing allocations; precompile wildcard patterns once at startup in DefaultBeaconChainMethods (e.g., build a List<Pair<String, Regex>> or a dedicated compiledPatterns field from allowedMethods where entries containing "*" are converted to Regex using pattern.replace("*", "[^/]+").toRegex()), then change isCallable to first check allowedMethods.contains(method) and then iterate the precompiled regex list (e.g., compiledPatterns.any { it.second.matches(method) }) to avoid per-request regex allocations.src/test/groovy/io/emeraldpay/dshackle/upstream/ethereum/EthereumLocalReaderSpec.groovy (1)
53-53: ⚡ Quick winStrengthen assertion to verify forwarded response content.
Line 53 only checks non-null, which can miss regressions in the returned payload. Assert the expected transaction hash to validate the behavior this test is targeting.
Proposed test assertion
- act != null + act != null + act.resultAsProcessedString == "0x0000000000000000000000000000000000000000000000000000000000000001"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/groovy/io/emeraldpay/dshackle/upstream/ethereum/EthereumLocalReaderSpec.groovy` at line 53, Replace the weak null check on the forwarded response (the lone "act != null" assertion) with an assertion that verifies the returned payload contains the expected transaction hash; locate the test in EthereumLocalReaderSpec where the response is stored in the variable act and compare act's transaction hash field against the expected hash value (use the same expectedTxHash/fixture used to set up the stubbed response) so the test validates the actual forwarded content rather than only non-nullness.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/kotlin/io/emeraldpay/dshackle/upstream/generic/GenericUpstream.kt (1)
367-373: ⚡ Quick winRe-evaluate the need for
pendingTxEvents()error handling; considerdistinctUntilChanged()instead.
connector.pendingTxEvents()comes fromPendingTransactionValidatorImpl.pendingTxExists(), which already converts failures into values via.timeout(...).onErrorResume { ... Mono.just(false) }(with logging). So this stream shouldn’t error, making the lack of anonErrorconsumer insubscribe { ... }unnecessary for the current implementation. The worthwhile low-effort improvement here is addingdistinctUntilChanged()to avoid emittingUPDATEDevery time the boolean remains unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/generic/GenericUpstream.kt` around lines 367 - 373, The pendingTxEvents() stream is already resilient to errors, so add distinctUntilChanged() to connector.pendingTxEvents() before subscribing to avoid emitting sendUpstreamStateEvent(UPDATED) when the boolean hasn't changed; update the subscription on pendingTxSubscription to subscribe to connector.pendingTxEvents().distinctUntilChanged() and keep the existing subscriber body that sets hasPendingTxs and calls sendUpstreamStateEvent(UPDATED) (no separate onError handler needed).src/main/kotlin/io/emeraldpay/dshackle/upstream/ethereum/PendingTransactionValidator.kt (1)
38-64: ⚡ Quick winDon’t change
flatMaptoconcatMapfortxpool_contenthere—current config won’t overlap
PendingTransactionValidatorImpl.pendingTxExists()bounds eachtxpool_contentcall with.timeout(Duration.ofSeconds(30)), while the productionintervalpassed in isDuration.ofMinutes(5)(RPC connector) /Duration.ofMinutes(10)(WS connector), so the nextFlux.intervaltick can’t start before the previous call finishes or times out.concatMapwould only be relevant if the polling interval could ever be ≤ 30s.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/ethereum/PendingTransactionValidator.kt` around lines 38 - 64, Do not change the reactive operator in PendingTransactionValidatorImpl.pendingTxExists() — keep the flatMap used for the ChainRequest("txpool_content", ...) call; the call is already guarded by .timeout(Duration.ofSeconds(30)) and production polling intervals (Duration.ofMinutes(5)/Duration.ofMinutes(10)) are much larger than the 30s timeout so there is no overlap and concatMap is unnecessary; leave the timeout and .onErrorResume(...) behavior intact and do not replace flatMap with concatMap.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/kotlin/io/emeraldpay/dshackle/rpc/ChainEventMapper.kt`:
- Around line 35-41: The code builds caps by mapping capabilities with a branch
that returns null for unmatched cases, which leaves nulls that crash protobuf's
addAllCapabilities; in ChainEventMapper.kt change the transformation to filter
out nulls (use mapNotNull or filter+map) when creating caps from capabilities
and remove the else -> null branch so the Iterable passed to addAllCapabilities
contains only non-null BlockchainOuterClass.Capabilities values (locate the
capabilities variable and the caps assignment).
---
Nitpick comments:
In
`@src/main/kotlin/io/emeraldpay/dshackle/upstream/ethereum/PendingTransactionValidator.kt`:
- Around line 38-64: Do not change the reactive operator in
PendingTransactionValidatorImpl.pendingTxExists() — keep the flatMap used for
the ChainRequest("txpool_content", ...) call; the call is already guarded by
.timeout(Duration.ofSeconds(30)) and production polling intervals
(Duration.ofMinutes(5)/Duration.ofMinutes(10)) are much larger than the 30s
timeout so there is no overlap and concatMap is unnecessary; leave the timeout
and .onErrorResume(...) behavior intact and do not replace flatMap with
concatMap.
In `@src/main/kotlin/io/emeraldpay/dshackle/upstream/generic/GenericUpstream.kt`:
- Around line 367-373: The pendingTxEvents() stream is already resilient to
errors, so add distinctUntilChanged() to connector.pendingTxEvents() before
subscribing to avoid emitting sendUpstreamStateEvent(UPDATED) when the boolean
hasn't changed; update the subscription on pendingTxSubscription to subscribe to
connector.pendingTxEvents().distinctUntilChanged() and keep the existing
subscriber body that sets hasPendingTxs and calls
sendUpstreamStateEvent(UPDATED) (no separate onError handler needed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74e3b539-f738-4ad9-9d42-083185c4ab0e
📒 Files selected for processing (15)
foundation/src/main/kotlin/io/emeraldpay/dshackle/foundation/ChainOptions.ktfoundation/src/main/kotlin/io/emeraldpay/dshackle/foundation/ChainOptionsReader.ktsrc/main/kotlin/io/emeraldpay/dshackle/rpc/ChainEventMapper.ktsrc/main/kotlin/io/emeraldpay/dshackle/rpc/Describe.ktsrc/main/kotlin/io/emeraldpay/dshackle/upstream/Capability.ktsrc/main/kotlin/io/emeraldpay/dshackle/upstream/ethereum/EthereumEgressSubscription.ktsrc/main/kotlin/io/emeraldpay/dshackle/upstream/ethereum/PendingTransactionValidator.ktsrc/main/kotlin/io/emeraldpay/dshackle/upstream/generic/GenericUpstream.ktsrc/main/kotlin/io/emeraldpay/dshackle/upstream/generic/connectors/GenericConnector.ktsrc/main/kotlin/io/emeraldpay/dshackle/upstream/generic/connectors/GenericRpcConnector.ktsrc/main/kotlin/io/emeraldpay/dshackle/upstream/generic/connectors/GenericWsConnector.ktsrc/test/groovy/io/emeraldpay/dshackle/config/UpstreamsConfigReaderSpec.groovysrc/test/groovy/io/emeraldpay/dshackle/test/GenericConnectorMock.groovysrc/test/groovy/io/emeraldpay/dshackle/upstream/ethereum/EthereumEgressSubscriptionSpec.groovysrc/test/kotlin/io/emeraldpay/dshackle/upstream/ethereum/PendingTransactionValidatorTest.kt
✅ Files skipped from review due to trivial changes (1)
- src/test/groovy/io/emeraldpay/dshackle/config/UpstreamsConfigReaderSpec.groovy
| val caps = capabilities.filter { it != Capability.WS_PENDING_TX }.map { | ||
| when (it) { | ||
| Capability.RPC -> BlockchainOuterClass.Capabilities.CAP_CALLS | ||
| Capability.BALANCE -> BlockchainOuterClass.Capabilities.CAP_BALANCE | ||
| Capability.WS_HEAD -> BlockchainOuterClass.Capabilities.CAP_WS_HEAD | ||
| else -> null | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and show the relevant section around the reported lines
FILE="src/main/kotlin/io/emeraldpay/dshackle/rpc/ChainEventMapper.kt"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE" >&2
exit 1
fi
echo "---- File header (first 120 lines) ----"
sed -n '1,120p' "$FILE" | cat -n
echo "---- Lines 25-80 (focus area) ----"
sed -n '25,80p' "$FILE" | cat -n
# Find where addAllCapabilities is called in this file
echo "---- addAllCapabilities call sites in file ----"
rg -n "addAllCapabilities" "$FILE" || true
# Show the surrounding code of the first addAllCapabilities occurrence
LINE=$(rg -n "addAllCapabilities" "$FILE" -n | head -n1 | cut -d: -f1 || true)
if [ -n "${LINE:-}" ]; then
START=$((LINE-20))
END=$((LINE+20))
echo "---- Context around addAllCapabilities at line $LINE ----"
sed -n "${START},${END}p" "$FILE" | cat -n
fiRepository: LiskHQ/dshackle
Length of output: 10462
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Search for addAllCapabilities usages across repo ----"
rg -n "addAllCapabilities\(" . || true
echo "---- Locate any generated protobuf sources for BlockchainOuterClass ----"
# Try common locations; avoid assumptions by using fd for filename matches
fd -t f -i "BlockchainOuterClass" . || true
echo "---- Search for 'CapabilitiesEvent' definitions in repo ----"
rg -n "CapabilitiesEvent" . || true
echo "---- Search for 'message CapabilitiesEvent' in proto files (if any) ----"
fd -t f -eproto . || trueRepository: LiskHQ/dshackle
Length of output: 1837
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect Describe.kt around addAllCapabilities
echo "---- Describe.kt around addAllCapabilities ----"
sed -n '1,140p' src/main/kotlin/io/emeraldpay/dshackle/rpc/Describe.kt | cat -n
rg -n "addAllCapabilities" src/main/kotlin/io/emeraldpay/dshackle/rpc/Describe.kt -n
# Inspect MultistreamStateEvent and mapping inputs for mapCapabilities
echo "---- MultistreamStateEvent.kt ----"
sed -n '1,120p' src/main/kotlin/io/emeraldpay/dshackle/upstream/state/MultistreamStateEvent.kt | cat -n
echo "---- SubscribeChainStatus.kt (calls mapCapabilities) ----"
sed -n '1,120p' src/main/kotlin/io/emeraldpay/dshackle/rpc/SubscribeChainStatus.kt | cat -n
# Inspect cache.proto for any CapabilitiesEvent proto definition
echo "---- cache.proto ----"
sed -n '1,220p' src/main/proto/cache.proto | cat -nRepository: LiskHQ/dshackle
Length of output: 13986
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- Search for proto definitions mentioning CapabilitiesEvent/capabilities ----"
rg -n "CapabilitiesEvent|addAllCapabilities|capabilities" src/main/proto || true
echo "---- List proto files under src/main/proto ----"
fd -t f -eproto src/main/proto . || trueRepository: LiskHQ/dshackle
Length of output: 568
🌐 Web query:
protobuf java generated builder addAll repeated enum Iterable contains null behavior
💡 Result:
In Java, Protobuf generated code is designed to be null-safe and does not support null values for elements within repeated fields [1]. If you pass an Iterable containing null elements to a generated addAll<Field> method (such as addAllFoos), the operation will fail with a NullPointerException [2][1]. This is because the Protobuf library explicitly checks for null values when iterating through the provided collection to maintain the integrity of the generated message [2]. Specifically: - The addAll methods generated by the Protobuf compiler for repeated fields (including enums) iterate over the provided Iterable [3][2]. - During this iteration, the library performs a null check on each element [2]. If a null element is encountered, the operation throws a NullPointerException or a similar validation error, preventing the null value from being added to the internal list [2]. - Protobuf API methods in Java generally neither accept nor return nulls [1]. If you are encountering issues, ensure that the collection passed to the addAll method is filtered to remove any potential null elements before passing it to the Protobuf builder [2]. Note also that attempting to add an UNRECOGNIZED enum value to a repeated field may lead to exceptions, such as IllegalArgumentException, depending on the specific Protobuf version and implementation details [4].
Citations:
- 1: https://stackoverflow.com/questions/20891425/can-java-protocol-buffers-ever-return-null-on-the-list-getter-of-a-repeated-fiel
- 2: https://github.com/protocolbuffers/protobuf/blob/master/java/core/src/main/java/com/google/protobuf/RepeatedFieldBuilder.java
- 3: https://protobuf.dev/reference/java/java-generated/
- 4: Java addAll for list of enums throws without being annotated as
@Throwsprotocolbuffers/protobuf#22829
Avoid nullable entries when populating protobuf addAllCapabilities (src/main/kotlin/io/emeraldpay/dshackle/rpc/ChainEventMapper.kt:35-41)
else -> null makes caps contain null elements, and protobuf Java addAllCapabilities(...) throws at runtime when the iterable includes nulls. Filter them out with mapNotNull.
Suggested fix
- val caps = capabilities.filter { it != Capability.WS_PENDING_TX }.map {
+ val caps = capabilities
+ .filter { it != Capability.WS_PENDING_TX }
+ .mapNotNull {
when (it) {
Capability.RPC -> BlockchainOuterClass.Capabilities.CAP_CALLS
Capability.BALANCE -> BlockchainOuterClass.Capabilities.CAP_BALANCE
Capability.WS_HEAD -> BlockchainOuterClass.Capabilities.CAP_WS_HEAD
else -> null
}
- }
+ }📝 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.
| val caps = capabilities.filter { it != Capability.WS_PENDING_TX }.map { | |
| when (it) { | |
| Capability.RPC -> BlockchainOuterClass.Capabilities.CAP_CALLS | |
| Capability.BALANCE -> BlockchainOuterClass.Capabilities.CAP_BALANCE | |
| Capability.WS_HEAD -> BlockchainOuterClass.Capabilities.CAP_WS_HEAD | |
| else -> null | |
| } | |
| val caps = capabilities | |
| .filter { it != Capability.WS_PENDING_TX } | |
| .mapNotNull { | |
| when (it) { | |
| Capability.RPC -> BlockchainOuterClass.Capabilities.CAP_CALLS | |
| Capability.BALANCE -> BlockchainOuterClass.Capabilities.CAP_BALANCE | |
| Capability.WS_HEAD -> BlockchainOuterClass.Capabilities.CAP_WS_HEAD | |
| else -> null | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/kotlin/io/emeraldpay/dshackle/rpc/ChainEventMapper.kt` around lines
35 - 41, The code builds caps by mapping capabilities with a branch that returns
null for unmatched cases, which leaves nulls that crash protobuf's
addAllCapabilities; in ChainEventMapper.kt change the transformation to filter
out nulls (use mapNotNull or filter+map) when creating caps from capabilities
and remove the else -> null branch so the Iterable passed to addAllCapabilities
contains only non-null BlockchainOuterClass.Capabilities values (locate the
capabilities variable and the caps assignment).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@emerald-grpc`:
- Line 1: The PR only updates the emerald-grpc submodule from commit
1698b6f9e332548fe263098f877cd8e08c9963f8 to
ad144230e42b6ab929f796beb4d04069f935f6a7 but the old ref cannot be fetched;
fetch the missing commit history for the emerald-grpc submodule (so you can
inspect the full commit range), then review the diff between the two commits
(1698b6f9… → ad144230e…) and the upstream PR `#865` for the “kite
mainnet”/“robinhood mainnet” changes, confirm there are no breaking
API/compatibility changes affecting callers in our repo (search for references
to emerald-grpc symbols and generated grpc interfaces), run the full test suite
and any integration tests that exercise gRPC interactions, and update the PR
with a brief compatibility/test summary or revert the submodule bump if
incompatibilities are found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2814fd6e-9b29-456a-baac-7254dc53c2fd
📒 Files selected for processing (2)
emerald-grpcfoundation/src/main/resources/public
🚧 Files skipped from review as they are similar to previous changes (1)
- foundation/src/main/resources/public
* add Humanity chain (grpcId 1167) Bump emerald-grpc and foundation public submodules to include CHAIN_HUMANITY__MAINNET. Chain.kt is generated at build time. * add Humanity testnet (grpcId 10202) bump emerald-grpc and foundation public submodules --------- Co-authored-by: Claude <noreply@anthropic.com>
* fix(hyperliquid): derive native-tx routing labels from the ?hl= URL flag The include_hl_native_tx/exclude_hl_native_tx detector classified a node by scanning the last 300 blocks for a system (native) topup tx from a per-chain address. That cannot work on testnet: - the configured testnet address 0x6ed35e7d6de4b45f4efb8a91eff31afa49362569 was a regular bot (non-zero gasPrice, not filtered by hl-compliant mode, present on both node types); - real testnet system txs (from 0x2222...) are far too sparse and bursty (median gap ~1200 blocks, max ~9000 = ~2.5h at ~1s/block) for any practical window; - eth_getLogs is identical between compliant and non-compliant modes, so there is no cheap wide-range signal either. Our hl-node upstreams already encode the mode in the URL (?hl=false serves native txs, ?hl=true is compliant). Read that flag directly: - GenericUpstream captures the configured RPC/WS URL and exposes getRpcConnectionUrl(); - detectHlNativeTx emits include/exclude_hl_native_tx straight from ?hl= when present (cheap, exact, drift-free), for both mainnet and testnet; - it falls back to the recent-blocks scan only when there is no ?hl= flag, and only on mainnet (testnet without the flag is too sparse to classify); - the bogus HL_NATIVE_TX_FROM_TESTNET constant is removed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Fix hl --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough📝 WalkthroughThis pull request introduces extensive changes including adding a Dependabot configuration for automated dependency updates. It updates Gradle and GitHub Actions workflows to newer versions, modifies Docker base image, and enhances Makefile targets. The codebase removes the previous cryptographic ECDSA signer and replaces it with an RSA-based signer with configuration-driven injection. The signing infrastructure is reworked, removing Spring Cloud Sleuth tracing and span propagation integrations throughout request readers and RPC calls to eliminate tracing dependencies. It adds support for custom HTTP headers in upstream requests and new blockchain type support (AVM and Aztec) along with their corresponding chain-specific logic and validations. Further, it introduces manual lower-bound configuration support for upstreams, enriches cache and reactive stream types with nullability constraints, adds pending transaction validation for Ethereum, and enhances reactor scheduling and metrics instrumentation. Numerous imports and Spring annotations are migrated from Overall, this PR reorganizes signing, tracing, configuration, and chain-specific logic, modernizes dependencies, and improves robustness and observability throughout the codebase. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Merge latest changes from upstream repo
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests