-
Notifications
You must be signed in to change notification settings - Fork 37
[Breaking Change] Add --multisig-code-hash
flag to specify MultisigScript::V2
or MultisigScript::Legacy
#631
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
[Breaking Change] Add --multisig-code-hash
flag to specify MultisigScript::V2
or MultisigScript::Legacy
#631
Conversation
d8901d3
to
e83c169
Compare
eee67fe
to
64047ae
Compare
d4857cb
to
1346b72
Compare
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
51da577
to
ae0f57f
Compare
6c92435
to
d361b08
Compare
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.
Pull Request Overview
This PR upgrades multisig support by replacing the old MULTISIG_TYPE_HASH
constant with a new MultisigScript
enum, adds a --multisig-code-hash
flag to relevant CLI commands, and bumps ckb-sdk
to v4.0.0.
- Swap out
MULTISIG_TYPE_HASH
forMultisigScript::{Legacy, V2}
checks across code paths. - Introduce
arg_multisig_code_hash
parser and require--multisig-code-hash
in wallet, tx, and util subcommands. - Update all
ckb-sdk
dependencies from 3.7.0 to 4.0.0.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
test/src/spec/wallet.rs | Adjusted multisig address test to include --multisig-code-hash legacy . |
Cargo.toml (root, test, signer) | Bumped ckb-sdk versions to 4.0.0 . |
src/utils/tx_helper.rs | Replaced constant checks with MultisigScript -based lookups. |
src/subcommands/mod.rs | Added definition of arg_multisig_code_hash . |
src/subcommands/tx.rs | Wired --multisig-code-hash into tx subcommands and parsing. |
src/subcommands/util.rs | Integrated code-hash flag into util commands and gen_multisig_addr . |
Comments suppressed due to low confidence (2)
test/src/spec/wallet.rs:221
- [nitpick] This test only covers the
legacy
script variant. Consider adding a case for the default V2 code hash to ensure both variants are exercised.
r#"util to-multisig-addr --sighash-address {} --locktime "2020-01-02T21:00:00+08:00" --multisig-code-hash legacy"#
src/subcommands/mod.rs:115
- The help text mentions a 'default' v2 code hash, but the flag is marked required, so there's no actual default. Consider either removing the default note or adding
.default_value("v2")
to the argument definition.
.about("Specifies the multisig code hash to use:\n - v2(default): ...")
MultisigScript::V2
and MultisigScript::Legacy
d361b08
to
12f3142
Compare
52553f1
to
ba5e678
Compare
Invite @quake @doitian @driftluo @zhangsoledad to review again. |
MultisigScript::V2
and MultisigScript::Legacy
--multisig-code-hash
arg to specify MultisigScript::V2
or MultisigScript::Legacy
--multisig-code-hash
arg to specify MultisigScript::V2
or MultisigScript::Legacy
--multisig-code-hash
argument to specify MultisigScript::V2
or MultisigScript::Legacy
--multisig-code-hash
argument to specify MultisigScript::V2
or MultisigScript::Legacy
--multisig-code-hash
flag to specify MultisigScript::V2
or MultisigScript::Legacy
ba5e678
to
03d7dc3
Compare
Signed-off-by: Eval EXEC <execvy@gmail.com>
03d7dc3
to
90c2ec0
Compare
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.
Pull Request Overview
This PR adds a --multisig-code-hash
flag to allow choosing between legacy and V2 multisig script hashes, updates configuration formats to include the new code hash field (defaulting to legacy for compatibility), and upgrades ckb-sdk
to v4.0.0.
- Upgraded
ckb-sdk
dependency and replacedMULTISIG_TYPE_HASH
constant withMultisigScript
enum usage. - Added
lock_code_hash
to multisig configs with backward-compat defaults. - Extended CLI and util commands to accept and propagate the new
--multisig-code-hash
flag.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
test/src/spec/wallet.rs | Updated test command to pass legacy code-hash flag |
test/Cargo.toml | Bumped ckb-sdk to v4.0.0 |
src/utils/tx_helper.rs | Handled both legacy and V2 multisig code hashes |
src/utils/rpc/types.rs | Adjusted script serialization suffixes for legacy/V2 |
src/utils/genesis_info.rs | Changed multisig_dep() to take a MultisigScript parameter |
src/utils/arg_parser.rs | Extended new_multisig() to accept script version |
src/subcommands/wallet.rs | Detected and matched legacy/V2 multisig configs |
src/subcommands/util.rs | Added flag but initial to-multisig-addr branch still hardcodes legacy |
src/subcommands/tx.rs | Integrated flag into tx subcommands and validators |
src/subcommands/mod.rs | Introduced arg_multisig_code_hash and parser helper |
src/subcommands/deploy/tx_builder.rs | Refactored deploy builder for versioned multisig deps |
ckb-signer/Cargo.toml | Bumped ckb-sdk to v4.0.0 |
Cargo.toml | Bumped ckb-sdk to v4.0.0 |
Comments suppressed due to low confidence (2)
src/subcommands/util.rs:691
- The
util to-multisig-addr
command always usesMultisigScript::Legacy
, ignoring the--multisig-code-hash
flag; parse the selected script viaarg_get_multisig_code_hash(m)
and pass it togen_multisig_addr
.
let (epoch_fraction, addr_payload) = gen_multisig_addr(MultisigScript::Legacy, address.payload(), None, elapsed);
test/src/spec/wallet.rs:221
- This spec only tests the legacy multisig code hash path; consider adding a test case for
--multisig-code-hash v2
to ensure V2 behavior is also validated.
r#"util to-multisig-addr --sighash-address {} --locktime "2020-01-02T21:00:00+08:00" --multisig-code-hash legacy"#
ckb-sdk-rust
dep: [BREAKING CHANGE] AddMultisigVersion::Legacy
andMultisigVersion::V2
ckb-sdk-rust#140lock-code-hash
field tomultisig_config
intx.json
. For backward compatibility, if this field is missing, it defaults to the legacy multisig script’s code hash (MultisigScript::Legacy
). This ensures oldertx.json
files remain valid and behave as expected. :--multisig-code-hash
arg to some commands, the--multisig-code-hash
flag help message isckb-cli util to-multisig-addr --multisig-code-hash v2 ...
❯ ./target/release/ckb-cli util to-multisig-addr --multisig-code-hash v2 --sighash-address ckt1qzda0cr08m85hc8jlnfp3zer7xulejywt49kt2rr0vthywaa50xwsqdfq3kxc87y7nq00c3mmxhy3yfzj7fpdzcktzr39 --locktime 2014-11-28T21:00:00+00:00 address: mainnet: ckb1qqmvjudc6s0mm992hjnhm367sfnjntycg3a5d7g7qpukz4wamvxjjq4jqqem772qpur6jw9vt59stawchrflq4s09vqgkpggqusqvm04rr testnet: ckt1qqmvjudc6s0mm992hjnhm367sfnjntycg3a5d7g7qpukz4wamvxjjq4jqqem772qpur6jw9vt59stawchrflq4s09vqgkpggqusqq3h6mr target_epoch: "Epoch { number: 11023, index: 1419, length: 1800 }"
ckb-cli tx build-multisig-address --multisig-code-hash v2 ...
ckb-cli tx add-multisig-config --multisig-code-hash v2 ...