feat: add wallet utilities and C-API improvements#200
Merged
fpelliccioni merged 1 commit intomasterfrom Mar 26, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds RIPEMD-160 C API hash, a new C outputpoint list type with converters and wiring, BCH token-aware address encoding (C++ and C API), script‑hash unlocking-placeholder factories, Conan lock/create option to disable tests, ccache usage in build scripts, and a minor token_data comment. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/domain/include/kth/domain/wallet/payment_address.hpp (1)
105-108: LGTM! Declaration follows existing patterns.The method declaration is properly guarded with
#if defined(KTH_CURRENCY_BCH)and uses[[nodiscard]]consistently with other encoding methods.Optional consideration: The relevant code snippets show that
encoded_legacy()andencoded_cashaddr()both have corresponding C API bindings inpayment_address.h. You may want to add akth_wallet_payment_address_encoded_token()binding for API consistency, though users can achieve the same result viakth_wallet_payment_address_encoded_cashaddr(addr, true).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/include/kth/domain/wallet/payment_address.hpp` around lines 105 - 108, Add a C API binding for the new encoded_token() C++ method to maintain API consistency: implement a kth_wallet_payment_address_encoded_token(...) function that mirrors the behavior of encoded_token() and follows the pattern used by kth_wallet_payment_address_encoded_legacy() / kth_wallet_payment_address_encoded_cashaddr(), delegating to payment_address::encoded_token() (or the corresponding wrapper) and returning a C string or error in the same way other encoded bindings do; ensure the new binding is guarded by the same KTH_CURRENCY_BCH preprocessor condition and included in the payment_address.h/C source pair.src/domain/src/chain/script_basis.cpp (1)
580-586: Add a defensive upper bound onscript_sizebefore allocating.This helper currently trusts
script_sizeentirely; a simple cap avoids accidental large allocations from upstream misuse.Proposed defensive guard
operation::list script_basis::to_pay_script_hash_pattern_unlocking_placeholder(size_t script_size) { + if (script_size > static_absolute_max_block_size()) { + return {}; + } data_chunk placeholder_script(script_size, 0); return operation::list { operation(opcode::push_size_0), operation(std::move(placeholder_script)) }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/src/chain/script_basis.cpp` around lines 580 - 586, The helper script_basis::to_pay_script_hash_pattern_unlocking_placeholder currently allocates data_chunk(script_size, 0) with an unchecked script_size; add a defensive upper bound by clamping or validating script_size before allocation (e.g., compare against a defined MAX_SCRIPT_PLACEHOLDER_SIZE constant and either cap it or return/error if exceeded) so that data_chunk(…) cannot allocate arbitrarily large memory; update the function to use the validated/clamped size when constructing placeholder_script and document the MAX_SCRIPT_PLACEHOLDER_SIZE constant near the function or in the header.src/domain/include/kth/domain/chain/script.hpp (1)
249-250: Clarify whatscript_sizerepresents in this API.A short contract comment here (e.g., “redeem-script raw bytes length”) will reduce caller ambiguity and avoid estimation drift.
Proposed doc clarification
static + // `script_size` is the raw redeem-script byte length (without outer scriptSig varint). operation::list to_pay_script_hash_pattern_unlocking_placeholder(size_t script_size);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/include/kth/domain/chain/script.hpp` around lines 249 - 250, Add a short contract comment for the function declaration to clarify what the script_size parameter means: indicate that in to_pay_script_hash_pattern_unlocking_placeholder(size_t script_size) the script_size represents the redeem script's raw byte length (i.e., the exact serialized size of the script being paid-to) and any expectations about whether it includes witness/version bytes or only scriptPubKey/redeemScript bytes; place this one-line description immediately above the function declaration to remove caller ambiguity and avoid size-estimation drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build-create-ems.sh`:
- Around line 15-18: The first conan lock create that generates conan-wasm.lock
is missing the option overrides, causing inconsistent lockfile options; update
the initial `conan lock create conanfile.py --version="${VERSION}"
--lockfile=conan-wasm.lock --update -pr ems2` invocation to include `-o
consensus=False -o tests=False` so it matches the subsequent `conan lock create
... --lockfile-out=build/conan.lock -pr ems2 -o consensus=False -o tests=False`
and the later `conan create` call, ensuring consistent dependency resolution and
lockfile contents.
In `@src/c-api/CMakeLists.txt`:
- Line 240: The CMake kth_headers list is missing the header corresponding to
the newly added source src/chain/outputpoint_list.cpp; update the kth_headers
list to include include/kth/capi/chain/outputpoint_list.h alongside the other
chain headers (e.g. point_list.h and output_list.h) so the header is installed
by cmake --install.
In `@src/c-api/src/chain/outputpoint_list.cpp`:
- Line 16: The list definition outputpoint_list stores
std::vector<kth::domain::chain::point> but the converter
kth_chain_output_point_const_cpp produces kth::domain::chain::output_point,
causing object slicing of output_point-specific fields; fix by making the
container hold std::vector<kth::domain::chain::output_point> and keep using
kth_chain_output_point_const_cpp, or alternatively change the converter to
kth_chain_point_const_cpp so the converter and stored element type match; update
the KTH_LIST_DEFINE invocation for outputpoint_list (and the typedef
kth_outputpoint_list_t) to use the matching element type/converter pair
(kth::domain::chain::output_point with kth_chain_output_point_const_cpp or
kth::domain::chain::point with kth_chain_point_const_cpp).
---
Nitpick comments:
In `@src/domain/include/kth/domain/chain/script.hpp`:
- Around line 249-250: Add a short contract comment for the function declaration
to clarify what the script_size parameter means: indicate that in
to_pay_script_hash_pattern_unlocking_placeholder(size_t script_size) the
script_size represents the redeem script's raw byte length (i.e., the exact
serialized size of the script being paid-to) and any expectations about whether
it includes witness/version bytes or only scriptPubKey/redeemScript bytes; place
this one-line description immediately above the function declaration to remove
caller ambiguity and avoid size-estimation drift.
In `@src/domain/include/kth/domain/wallet/payment_address.hpp`:
- Around line 105-108: Add a C API binding for the new encoded_token() C++
method to maintain API consistency: implement a
kth_wallet_payment_address_encoded_token(...) function that mirrors the behavior
of encoded_token() and follows the pattern used by
kth_wallet_payment_address_encoded_legacy() /
kth_wallet_payment_address_encoded_cashaddr(), delegating to
payment_address::encoded_token() (or the corresponding wrapper) and returning a
C string or error in the same way other encoded bindings do; ensure the new
binding is guarded by the same KTH_CURRENCY_BCH preprocessor condition and
included in the payment_address.h/C source pair.
In `@src/domain/src/chain/script_basis.cpp`:
- Around line 580-586: The helper
script_basis::to_pay_script_hash_pattern_unlocking_placeholder currently
allocates data_chunk(script_size, 0) with an unchecked script_size; add a
defensive upper bound by clamping or validating script_size before allocation
(e.g., compare against a defined MAX_SCRIPT_PLACEHOLDER_SIZE constant and either
cap it or return/error if exceeded) so that data_chunk(…) cannot allocate
arbitrarily large memory; update the function to use the validated/clamped size
when constructing placeholder_script and document the
MAX_SCRIPT_PLACEHOLDER_SIZE constant near the function or in the header.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d8b307d-8875-45a6-8f73-e748d40302f3
📒 Files selected for processing (15)
scripts/build-create-ems.shsrc/c-api/CMakeLists.txtsrc/c-api/include/kth/capi/chain/outputpoint_list.hsrc/c-api/include/kth/capi/conversions.hppsrc/c-api/include/kth/capi/hash.hsrc/c-api/include/kth/capi/primitives.hsrc/c-api/src/chain/outputpoint_list.cppsrc/c-api/src/hash.cppsrc/domain/include/kth/domain/chain/script.hppsrc/domain/include/kth/domain/chain/script_basis.hppsrc/domain/include/kth/domain/chain/token_data.hppsrc/domain/include/kth/domain/wallet/payment_address.hppsrc/domain/src/chain/script.cppsrc/domain/src/chain/script_basis.cppsrc/domain/src/wallet/payment_address.cpp
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #200 +/- ##
==========================================
- Coverage 47.61% 47.58% -0.03%
==========================================
Files 326 326
Lines 16066 16075 +9
Branches 5826 5829 +3
==========================================
Hits 7650 7650
- Misses 6242 6251 +9
Partials 2174 2174 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fpelliccioni
force-pushed
the
feat/wallet-utilities
branch
from
6aaccbb to
d295a3b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/domain/include/kth/domain/wallet/payment_address.hpp`:
- Around line 106-107: Add a C-API wrapper to expose
payment_address::encoded_token(): declare and implement a function named e.g.
kth_payment_address_encoded_token that accepts the C payment_address handle
(kth_payment_address_t const* or equivalent), checks for null, calls the C++
method payment_address::encoded_token(), converts/allocates the returned
std::string into the C string/return type used by the existing C-API (consistent
with other wrappers), and ensure ownership/cleanup semantics match other C-API
string-returning functions; add the declaration to the C-API header and the
forwarding implementation to the C-API source alongside the other
payment_address wrappers.
In `@src/domain/src/chain/script_basis.cpp`:
- Around line 580-586: The placeholder function
script_basis::to_pay_script_hash_pattern_unlocking_placeholder currently always
prepends opcode::push_size_0 (the multisig dummy OP_0) which inflates size/fee;
change the signature to accept an optional flag (e.g., bool
include_multisig_dummy = false) and only push opcode::push_size_0 when that flag
is true, otherwise return only the placeholder_script operation; update callers
that expect a multisig placeholder to pass true and leave others unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 64a575ea-6780-4520-9192-cb672dee2509
📒 Files selected for processing (15)
scripts/build-create-ems.shsrc/c-api/CMakeLists.txtsrc/c-api/include/kth/capi/chain/outputpoint_list.hsrc/c-api/include/kth/capi/conversions.hppsrc/c-api/include/kth/capi/hash.hsrc/c-api/include/kth/capi/primitives.hsrc/c-api/src/chain/outputpoint_list.cppsrc/c-api/src/hash.cppsrc/domain/include/kth/domain/chain/script.hppsrc/domain/include/kth/domain/chain/script_basis.hppsrc/domain/include/kth/domain/chain/token_data.hppsrc/domain/include/kth/domain/wallet/payment_address.hppsrc/domain/src/chain/script.cppsrc/domain/src/chain/script_basis.cppsrc/domain/src/wallet/payment_address.cpp
✅ Files skipped from review due to trivial changes (6)
- src/c-api/include/kth/capi/primitives.h
- src/c-api/include/kth/capi/chain/outputpoint_list.h
- src/c-api/src/hash.cpp
- src/c-api/CMakeLists.txt
- src/c-api/src/chain/outputpoint_list.cpp
- src/domain/include/kth/domain/chain/token_data.hpp
🚧 Files skipped from review as they are similar to previous changes (6)
- src/c-api/include/kth/capi/hash.h
- src/domain/include/kth/domain/chain/script.hpp
- src/c-api/include/kth/capi/conversions.hpp
- src/domain/src/chain/script.cpp
- src/domain/src/wallet/payment_address.cpp
- scripts/build-create-ems.sh
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
fpelliccioni
force-pushed
the
feat/wallet-utilities
branch
from
d295a3b to
a2151eb
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/domain/src/chain/script_basis.cpp (1)
580-586:⚠️ Potential issue | 🟠 MajorMake
OP_0optional in the generic P2SH unlocking placeholder.Line 583 unconditionally adds
opcode::push_size_0. That byte is only needed forCHECKMULTISIG-style redeem paths, so this overestimates size/fees for other P2SH scripts.Proposed fix
-operation::list script_basis::to_pay_script_hash_pattern_unlocking_placeholder(size_t script_size) { +operation::list script_basis::to_pay_script_hash_pattern_unlocking_placeholder(size_t script_size, bool include_multisig_dummy) { data_chunk placeholder_script(script_size, 0); - return operation::list { - operation(opcode::push_size_0), - operation(std::move(placeholder_script)) - }; + operation::list ops; + ops.reserve(include_multisig_dummy ? 2 : 1); + if (include_multisig_dummy) { + ops.emplace_back(opcode::push_size_0); + } + ops.emplace_back(std::move(placeholder_script)); + return ops; }Based on learnings: prior fee over-estimation in this repository already caused false
insufficient_amountoutcomes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/src/chain/script_basis.cpp` around lines 580 - 586, The current to_pay_script_hash_pattern_unlocking_placeholder unconditionally prepends opcode::push_size_0, which overestimates size for non-multisig P2SH; change the function signature (script_basis::to_pay_script_hash_pattern_unlocking_placeholder) to accept a boolean flag (e.g., include_op0 or is_multisig) defaulting to false, and only push opcode::push_size_0 into the returned operation::list when that flag is true; keep creating the placeholder_script and pushing it as before (operation(std::move(placeholder_script))), and update any callers to pass true where a CHECKMULTISIG-style redeem path is expected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/domain/src/chain/script_basis.cpp`:
- Around line 580-586: The current
to_pay_script_hash_pattern_unlocking_placeholder unconditionally prepends
opcode::push_size_0, which overestimates size for non-multisig P2SH; change the
function signature
(script_basis::to_pay_script_hash_pattern_unlocking_placeholder) to accept a
boolean flag (e.g., include_op0 or is_multisig) defaulting to false, and only
push opcode::push_size_0 into the returned operation::list when that flag is
true; keep creating the placeholder_script and pushing it as before
(operation(std::move(placeholder_script))), and update any callers to pass true
where a CHECKMULTISIG-style redeem path is expected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a027e6c0-fcf0-4d1b-a2c2-eec5c9406c17
📒 Files selected for processing (15)
scripts/build-create-ems.shsrc/c-api/CMakeLists.txtsrc/c-api/include/kth/capi/chain/outputpoint_list.hsrc/c-api/include/kth/capi/conversions.hppsrc/c-api/include/kth/capi/hash.hsrc/c-api/include/kth/capi/primitives.hsrc/c-api/src/chain/outputpoint_list.cppsrc/c-api/src/hash.cppsrc/domain/include/kth/domain/chain/script.hppsrc/domain/include/kth/domain/chain/script_basis.hppsrc/domain/include/kth/domain/chain/token_data.hppsrc/domain/include/kth/domain/wallet/payment_address.hppsrc/domain/src/chain/script.cppsrc/domain/src/chain/script_basis.cppsrc/domain/src/wallet/payment_address.cpp
✅ Files skipped from review due to trivial changes (2)
- src/c-api/include/kth/capi/chain/outputpoint_list.h
- src/c-api/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (10)
- src/domain/include/kth/domain/chain/token_data.hpp
- src/domain/include/kth/domain/chain/script_basis.hpp
- src/c-api/include/kth/capi/hash.h
- src/domain/include/kth/domain/chain/script.hpp
- src/domain/include/kth/domain/wallet/payment_address.hpp
- src/domain/src/chain/script.cpp
- scripts/build-create-ems.sh
- src/c-api/include/kth/capi/conversions.hpp
- src/domain/src/wallet/payment_address.cpp
- src/c-api/src/chain/outputpoint_list.cpp
fpelliccioni
force-pushed
the
feat/wallet-utilities
branch
from
a2151eb to
53d0887
Compare
- Add P2SH unlocking script placeholder for transaction size estimation - Add encoded_token() method on payment_address for CashToken addresses - Expose kth_ripemd160_hash() in C-API - Add outputpoint_list type to C-API - Skip tests in Emscripten WASM builds (build-create-ems.sh) - Clarify token_data id field documentation
fpelliccioni
force-pushed
the
feat/wallet-utilities
branch
from
53d0887 to
91308d6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/rebuild.sh (1)
33-43: LGTM! Consistent with build.sh.The ccache integration mirrors the changes in
build.sh, which is good for consistency.Note: Both
build.shandrebuild.shnow share identical ccache detection logic (lines 33-36). If these scripts continue to diverge, consider extracting common utilities to a shared helper script (e.g.,scripts/common.sh) to reduce maintenance burden. This is optional for now given the small scope.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/rebuild.sh` around lines 33 - 43, Duplicate ccache-detection logic (the CCACHE_OPT variable and the `command -v ccache` check) exists in rebuild.sh and build.sh; extract this into a shared helper (eg. a new function detect_ccache or set_ccache_opts in a common script) and have both scripts source that helper so they set CCACHE_OPT consistently, then ensure the cmake invocation still uses $CCACHE_OPT; update rebuild.sh to remove the inline detection and call the shared function/variable instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/rebuild.sh`:
- Around line 33-43: Duplicate ccache-detection logic (the CCACHE_OPT variable
and the `command -v ccache` check) exists in rebuild.sh and build.sh; extract
this into a shared helper (eg. a new function detect_ccache or set_ccache_opts
in a common script) and have both scripts source that helper so they set
CCACHE_OPT consistently, then ensure the cmake invocation still uses
$CCACHE_OPT; update rebuild.sh to remove the inline detection and call the
shared function/variable instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14185056-9630-41fd-b017-b3bfccc06879
📒 Files selected for processing (20)
scripts/build-create-ems.shscripts/build.shscripts/rebuild.shsrc/c-api/CMakeLists.txtsrc/c-api/include/kth/capi/capi.hsrc/c-api/include/kth/capi/chain/outputpoint_list.hsrc/c-api/include/kth/capi/conversions.hppsrc/c-api/include/kth/capi/hash.hsrc/c-api/include/kth/capi/primitives.hsrc/c-api/include/kth/capi/wallet/payment_address.hsrc/c-api/src/chain/outputpoint_list.cppsrc/c-api/src/hash.cppsrc/c-api/src/wallet/payment_address.cppsrc/domain/include/kth/domain/chain/script.hppsrc/domain/include/kth/domain/chain/script_basis.hppsrc/domain/include/kth/domain/chain/token_data.hppsrc/domain/include/kth/domain/wallet/payment_address.hppsrc/domain/src/chain/script.cppsrc/domain/src/chain/script_basis.cppsrc/domain/src/wallet/payment_address.cpp
✅ Files skipped from review due to trivial changes (5)
- src/c-api/include/kth/capi/capi.h
- src/c-api/include/kth/capi/primitives.h
- src/domain/include/kth/domain/chain/token_data.hpp
- src/c-api/include/kth/capi/chain/outputpoint_list.h
- src/c-api/src/chain/outputpoint_list.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- scripts/build-create-ems.sh
- src/domain/src/wallet/payment_address.cpp
- src/c-api/include/kth/capi/conversions.hpp
- src/domain/include/kth/domain/chain/script.hpp
- src/c-api/include/kth/capi/hash.h
- src/c-api/CMakeLists.txt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
encoded_token()method onpayment_addressfor CashToken-aware addresseskth_ripemd160_hash()in C-APIoutputpoint_listtype to C-APIbuild-create-ems.sh)token_dataid field documentationTest plan
build-create-ems.shencoded_token()returns correct CashToken address formatkth_ripemd160_hash()produces correct hashesNote
Medium Risk
Medium risk because it adds new wallet address encoding and script-construction helpers that affect transaction sizing/serialization, plus expands the public C-API surface. Changes are mostly additive, but downstream integrations may need updates and outputs should be validated for correctness.
Overview
Adds several new C-API capabilities:
kth_ripemd160_hash(), a newkth_outputpoint_list_tlist type (wired intoCMakeLists.txt,primitives.h,capi.h, and conversion helpers), and a BCH-onlykth_wallet_payment_address_encoded_token()wrapper.Extends domain wallet/script utilities by adding
payment_address::encoded_token()(CashToken-aware CashAddr) and a newscript(_basis)::to_pay_script_hash_pattern_unlocking_placeholder(script_size, multisig)helper for building placeholder P2SH unlocking scripts (with optional CHECKMULTISIG dummy) useful for size estimation. Build tooling is updated to skip tests in Emscripten Conan builds and to auto-enableccachein nativebuild.sh/rebuild.sh;token_data_t::idis clarified with a comment.Written by Cursor Bugbot for commit 91308d6. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Chores