-
Notifications
You must be signed in to change notification settings - Fork 258
feat: use akash forks of ledger repos #2022
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughBuild configuration changes: system dependency installation for libudev/dev added to CI and Goreleaser hooks, static linking (--static) removed from extldflags for linux/amd64 and linux/arm64, Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / CI
participant Hook as Goreleaser before.hooks
participant Build as Go build / goreleaser
participant Art as Artifact
Note over Dev,Hook: New/modified build steps
Dev->>Hook: trigger build
Hook->>Hook: apt update\napt install pkg-config libudev-dev (amd64/arm64)
Hook-->>Build: start build (extldflags: "-lc -lrt -lpthread")
Note right of Build: `--static` removed\n`hidraw` tag included
Build->>Art: produce binaries/images
Art-->>Dev: artifacts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.goreleaser-docker.yaml (1)
26-26: Ensure Docker base image includes required dynamic libraries.The removal of
--staticflag is consistent with.goreleaser.yaml, but verify that the Docker base image used in_build/Dockerfile.akashincludes the necessary dynamic libraries (libc, libpthread, librt) for the binary to run.Also applies to: 45-45
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
.github/actions/setup-ubuntu/action.yaml(1 hunks).goreleaser-docker.yaml(2 hunks).goreleaser-test-bins.yaml(3 hunks).goreleaser.yaml(2 hunks)Makefile(1 hunks)go.mod(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: network-upgrade
- GitHub Check: lint-go
- GitHub Check: release-dry-run
- GitHub Check: sims
- GitHub Check: coverage
- GitHub Check: tests
- GitHub Check: build-bins
🔇 Additional comments (8)
.goreleaser.yaml (1)
63-63: Verify runtime implications of dynamic linking and document deployment requirements.Removing the
--staticflag switches these binaries from static to dynamic linking. This means:
- Binaries will require system libraries (libc, libpthread, librt) at runtime
- Deployment environments must have these libraries available
- This is consistent with enabling HID support via
hidrawtag (see Makefile)Confirm that runtime environments (production, test, deployment pipelines) have the necessary dynamic dependencies available, and update deployment documentation accordingly.
Also applies to: 82-82
.goreleaser-test-bins.yaml (2)
5-8: Verify before-hooks execute with proper permissions in build environment.The new
beforehooks section runsapt updateand installs cross-compilation packages for both amd64 and arm64. This assumes:
- The goreleaser build environment runs with sufficient permissions (sudo or root)
- APT package repositories are available and reachable
- Multi-architecture packages (
libudev-dev:amd64andlibudev-dev:arm64) are available in configured repositoriesConfirm these assumptions hold in your CI/build environment, and consider adding error handling if APT commands fail.
67-67: Consistent with dynamic linking changes across goreleaser configs.The removal of
--staticflag is consistent with.goreleaser.yamland.goreleaser-docker.yaml. Combined with thebeforehooks installing libudev-dev, this ensures the build environment has all dependencies needed for dynamic HID-related linking.Also applies to: 86-86
go.mod (3)
32-32: Review direct dependency version changes for compatibility.Multiple direct dependencies were updated:
prometheus/client_golangv1.23.0 (line 32)testifyv1.11.1 (line 40)golang.org/x/modv0.26.0 (line 42)google.golang.org/grpcv1.75.0 (line 45)Verify that these version upgrades don't introduce breaking changes in your codebase. In particular, check:
- gRPC v1.75.0 - Confirm compatibility with existing gRPC service definitions and interceptors
- golang.org/x/mod v0.26.0 - Check if any public APIs used in the codebase have changed
Also applies to: 40-40, 42-42, 45-45
67-67: These fork migrations appear well-managed, but runtime validation is essential.Verification shows the ledger/HID fork changes are intentional and coordinated:
- Explicit commit:
3762e9a feat: use akash forks of ledger repos- Part of cosmos-sdk v0.53.x migration sequence
- Fixed versions in go.mod (v0.16.0) prevent surprise updates
- Both forks actively maintained (last updated today)
- These are transitive dependencies only (no direct imports in codebase)
The replace directives with explicit versions provide version stability. However, since these are transitive dependencies pulled through other packages, you should:
- Run integration tests to ensure the fork implementations don't introduce runtime issues
- Test with actual Ledger hardware if your application supports Ledger signing
- Verify the troian/hid fork works correctly with your USB/HID device interactions
The Akash Network fork strategy for critical dependencies is well-established (gogoproto, cometbft, cosmos-sdk). This appears consistent with that pattern, but only runtime testing can confirm drop-in compatibility with your use cases.
88-296: Verification complete: indirect dependency updates are secure and intentional.The git diff confirms these updates are intentional—commit message explicitly states "feat: bump ledger dependencies for latest fw support (#2010)." Security verification found no unpatched CVEs in the checked packages:
- google.golang.org/grpc v1.75.0 is not vulnerable to CVE-2025-55163 (that CVE is Java/Netty-specific)
- prometheus-client-golang v1.23.0 has no published CVE for that version
- go-jose v4.1.1 is beyond the fixed version for CVE-2025-27144 (fixed in v4.0.5)
License compliance verification requires standard operational/procurement review, not code-level inspection. The dependency updates appear to be recent, maintained versions and part of coordinated feature work.
Makefile (1)
29-29: ---Verify that macOS/Windows builds succeed with the hidraw build tag.
The goreleaser configurations pass the same
BUILD_TAGSenvironment variable (containinghidraw) to all platforms, including macOS and Windows builds. While the tag configuration itself is correct, hidraw is Linux-specific and the build may fail if thezondax/hiddependency (line 76 in go.mod) lacks proper platform guards.Confirm:
- macOS builds complete successfully: Run
make build-dockeror equivalent to verify darwin-amd64 and darwin-arm64 targets compile without errors related to missing hidraw headers- The
zondax/hidpackage properly guards hidraw code with// +build linuxconstraints.github/actions/setup-ubuntu/action.yaml (1)
17-17: Addition of libudev-dev to setup-ubuntu is correct and necessary.The hidraw build tag requires libudev-dev headers at compile time, and since the Makefile includes
hidrawin the defaultBUILD_TAGS, this dependency must be available in the standard Ubuntu setup workflow. The change is properly scoped and necessary for all builds using the hidraw tag.Regarding pkg-config: while pkg-config is not strictly required for the hidraw backend—the key requirement is the libudev development package itself. Since the Go build wrapper appears to work with just the libudev-dev headers installed, adding pkg-config is not necessary at this point.
Signed-off-by: Artur Troian <troian@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
go.mod (2)
89-281: Review extensive indirect dependency updates for compatibility.Numerous indirect dependencies have been updated, including:
- OpenTelemetry instrumentation packages (v0.61.0, v0.62.0, v1.37.0 core)
- Prometheus packages (client_model v0.6.2, common v0.65.0, procfs v0.16.1)
- Go standard library modules (crypto, net, sys, term, text, time)
- Google API and protobuf versions
These extensive updates are appropriate for maintaining security and compatibility but warrant verification that:
- All indirect dependencies are compatible with each other
- No breaking changes affect application functionality
- Unit and integration tests pass with these versions
Ensure comprehensive testing is performed to validate:
- All unit tests pass with the updated dependency versions
- Integration tests validate the new ledger library integrations
- Performance benchmarks remain stable
1-3: Verify hidraw build tag configuration.The AI-generated summary indicates a
hidrawbuild tag has been added to the Makefile to support the new HID library integration, but the Makefile was not provided for review. Ensure that:
- The
hidrawtag is properly added to BUILD_TAGS in the Makefile- The tag is included in all relevant build configurations (local builds, goreleaser, test-bins)
- Code conditionally compiled with this tag is properly tested
Please provide the Makefile changes or generate a script to verify the hidraw build tag configuration.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
.github/actions/setup-ubuntu/action.yaml(1 hunks).goreleaser-docker.yaml(3 hunks).goreleaser-test-bins.yaml(3 hunks).goreleaser.yaml(3 hunks)Makefile(1 hunks)go.mod(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- .goreleaser-docker.yaml
- .goreleaser-test-bins.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: release-dry-run
- GitHub Check: lint-go
- GitHub Check: tests
- GitHub Check: network-upgrade-names
- GitHub Check: coverage
- GitHub Check: build-bins
- GitHub Check: lint-shell
- GitHub Check: sims
🔇 Additional comments (4)
.github/actions/setup-ubuntu/action.yaml (1)
17-17: LGTM!The addition of
libudev-devis necessary for HID support and aligns with parallel changes in goreleaser configuration and the ledger library integration..goreleaser.yaml (1)
8-11: LGTM!The before hooks correctly set up the cross-compilation environment for libudev-dev packages needed for HID support on both amd64 and arm64 architectures.
go.mod (2)
32-32: Verify direct dependency updates don't introduce breaking changes.Direct dependencies have been updated:
prometheus/client_golangv1.22.0 → v1.23.0stretchr/testifyv1.10.0 → v1.11.1golang.org/x/modv0.25.0 → v0.26.0google.golang.org/grpcv1.74.2 → v1.75.0While these appear to be minor/patch-level updates, verify that no breaking API changes are introduced in these versions.
Also applies to: 40-40, 42-42, 45-45
66-78: Ledger fork repositories are actively maintained and serve a documented purpose.Verification confirms the fork repositories are appropriate:
- Both
akash-network/ledger-goandtroian/hidreleases are accessible and versioned (v0.16.0, v0.14.0)- Recent activity shows deliberate upstream tracking and backporting of changes from
cosmos/ledger-cosmos-goandZondax/ledger-go- The fork exists to resolve Cosmos SDK / Ledger signing flow compatibility issues for AKT users—a known and legitimate use case
- Maintenance strategy is sound: team actively pulls and backports changes from upstream sources rather than abandoning the dependency
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.md