Skip to content
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

feat!: use delegate cell lock as rollup cell lock #940

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

blckngm
Copy link
Contributor

@blckngm blckngm commented Jan 11, 2023

Use delegate cell lock to support managing block producer key via e.g. multisig lock.

Delegate cell lock is a simple lock that delegates verification to another lock via a “delegate cell”. Delegate cell can be managed separately by e.g. a multisig lock.

@blckngm
Copy link
Contributor Author

blckngm commented Jan 11, 2023

Some gw-tools args have changed, see kicker PR:

Edit: it should be still compatible with current kicker

Edit: no, need to modify scripts-config.json

@blckngm blckngm force-pushed the dev-delegate-cell-lock branch 2 times, most recently from 60af8fb to 5439a33 Compare January 12, 2023 07:45
@blckngm blckngm marked this pull request as ready for review January 12, 2023 07:47
@blckngm blckngm force-pushed the dev-delegate-cell-lock branch 2 times, most recently from 8160759 to c36937f Compare January 16, 2023 03:28
crates/config/Cargo.toml Outdated Show resolved Hide resolved
@blckngm blckngm requested review from jjyr and zeroqn January 17, 2023 09:04
jjyr
jjyr previously approved these changes Jan 29, 2023
}

// Get expected type hash from args.
let expected_type_hash: [u8; 32] = args.try_into().map_err(|_| Error::InvalidArgs)?;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better when args can be longer than 32 bytes. Better for upgrading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is expected to be upgraded here? It's unlikely that the length of “type hash” will change.

.position(|type_hash| type_hash == Some(expected_type_hash))
.ok_or(Error::CellDepNotFound)?;
let data = load_cell_data(cell_dep_idx, Source::CellDep)?;
let expected_lock_hash_160: [u8; 20] =

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about requiring this data to:

  • has a length >= 20
  • and is a prefix of the lock hash

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it is fair enough to take the first 20 bytes. There is no benefit to accepting a variant length lock hash.

let args = script.as_reader().args().raw_data();

let witness_arg = load_witness_args(0, Source::GroupInput)?;
if witness_arg.as_reader().lock().is_some() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The witness is not used in script.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is discussed in the internal issue. If we delegate to e.g. secp256k1/blake160, the witnesses of this lock will not be checked by it, so we added some basic sanity checks on the first witness arg.

zeroqn
zeroqn previously approved these changes Feb 2, 2023
zeroqn
zeroqn previously approved these changes Feb 24, 2023
jjyr
jjyr previously approved these changes Feb 27, 2023
@jjyr jjyr enabled auto-merge February 27, 2023 02:22
@jjyr
Copy link
Collaborator

jjyr commented Feb 27, 2023

@sopium please resolve conflicts

@jjyr jjyr requested review from zeroqn and removed request for XuJiandong February 28, 2023 03:21
@jjyr jjyr disabled auto-merge February 28, 2023 03:25
@jjyr jjyr merged commit 9338940 into godwokenrises:develop Feb 28, 2023
@blckngm blckngm deleted the dev-delegate-cell-lock branch March 14, 2023 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants