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

[DRAFT] Feat vc vcell #14

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from
Open

[DRAFT] Feat vc vcell #14

wants to merge 10 commits into from

Conversation

tinnendo
Copy link

@tinnendo tinnendo commented Feb 4, 2025

tbd

…ipt.

* chore(contracts): Fix typo

* refactor(contracts): Extend ChannelAction struct to reflect changes on VC cell creation and termination. Adapt main() function.

* chore(contracts): Add custom errors
Move helper functions from entry.rs to channels.rs
Correct case for Dispute start and add build_vchannel_status function to initialize a VC state for a VC cell, given data from PCTS.
…us type

* refactor(contracts): Move utility functions from entry.rs files to channels.rs, remove comments
@tinnendo tinnendo requested a review from janbormet February 4, 2025 14:09
Copy link
Collaborator

@janbormet janbormet left a comment

Choose a reason for hiding this comment

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

Review noch nicht fertig...

@@ -572,15 +550,22 @@ pub fn verify_status_not_funded(status: &ChannelStatus) -> Result<(), Error> {
Ok(())
}

pub fn verify_vc_status_not_disputed(status: &ChannelStatus) -> Result<(), Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this used?
A VC status should always be disputed no?

Comment on lines 89 to 98
// The perun-channel-lockscript (pcls) is used to lock access to interacting with a channel and is attached as lock script
// to the channel-cell (the cell which uses the perun-channel-type-script (pcts) as its type script).
// A channel defines two participants, each of which has their own unlock_script_hash (also defined in the ChannelConstants.params.{party_a,party_b}).
// The pcls allows a transaction to interact with the channel, if AT LEAST ONE INPUT CELL is present with:
// - cell's lock script hash == unlock_script_hash of party_a or
// - cell's lock script hash == unlock_script_hash of party_b
// We recommend using the secp256k1_blake160_sighash_all script as unlock script and corresponding payment args for the participants.
//
// Note: This means, that each participant needs to use a secp256k1_blake160_sighash_all as input to interact with the channel.
// This should not be a substantial restriction, since a payment input will likely be used anyway (e.g. for funding or fees).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Duplicate?

}
}

pub fn dispute_mode() -> Result<(), Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am pretty sure that we NEED to allow the intermediate to perform the merging.
This looks to me like only Alice and Bob can do it?

I think we can just have the lock script that always accepts for now...

// can be created on chain, as an OutPoint can only be consumed once.

// here, we verify that the OutPoint in the thread token is actually consumed.
verify_thread_token_integrity(&channel_constants.thread_token())?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Virtual channel cells should not have a thread token.
They gain their uniqueness through merging and the uniqueness of the parent cells.

// This check is not strictly necessary for the current implementation of the pfls, but it is good practice to
// verify this anyway, as there is no reason to include funds locked for any channel in the input of a transaction
// that creates a new channel besides trying some kind of attack.
verify_no_funds_in_inputs(channel_constants)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be removed. PFLS only unlock if PCTS is present. This is no concern of the VCTS.


// We verify that the state the channel starts with is valid according to the utxo-adaption of the perun protocol.
// For example, the channel must not be final and the version number must be 0.
verify_vcstate_valid_as_start(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could probably relax these requirements.
As far as I can see, virtual channels should not have a minimum balance requirement, as they are not directly funded on-chain.

debug!("verify_vcstate_valid_as_start passed");

// Here we verify that the first party completes its funding and that itsfunds are actually locked to the pfls with correct args.
verify_funding_in_outputs(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, Virtual channels are not funded...
So there should not be funds in outputs

debug!("verify_vc_funded_status passed");

// We verify that the channel status is not disputed upon start.
verify_vc_status_not_disputed(new_status)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The VCTS should only ever appear in a disputed state.

debug!("verify_vcfunding_in_outputs passed");

// We check that the funded bit in the channel status is set to true, exactly if the funding is complete.
verify_funded_vc_status(new_status, true)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here. I think VCStatus does not need to have a disputed or funded bit.

Err(Error::ChannelStateNotEqual)
}

pub fn verify_funding_in_outputs(
Copy link
Collaborator

Choose a reason for hiding this comment

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

virtual channels don't have direct on-chain funding. This should be removed

Changes made to contract to support vc cells
* open vc cell
* progress without update
* progress with update
* vc merge
* vc close 1
* vc close 2
* update types.mol and change contract for the updated types.
* chore: remove comments
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