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

Migrate to Plutus V3 #7

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open

Migrate to Plutus V3 #7

wants to merge 50 commits into from

Conversation

keyan-m
Copy link
Contributor

@keyan-m keyan-m commented Sep 17, 2024

No description provided.

The `collective_output` validation is now removed to avoid the
additional list reversal. Now the validation is only provided with the
number of outputs for each input.
It is less costly to look up a given withdrawal script than looking up a
spend's script hash.
They both now look into `redeemers` and their corresponding fields (i.e.
`mint` for tx-level pattern, and `withdrawals` staking pattern).

This gives users access to minted tokens and withdrawal amounts.
@keyan-m keyan-m marked this pull request as ready for review November 11, 2024 16:23
Copy link

@mpetruska mpetruska left a comment

Choose a reason for hiding this comment

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

Very nice work! Just some minor observations and questions.

use cardano/address.{Script}
use cardano/transaction.{Redeemer, ScriptPurpose, Withdraw}

/// Datatype for redeemer of the "computation stake validator," to represent

Choose a reason for hiding this comment

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

Suggested change
/// Datatype for redeemer of the "computation stake validator," to represent
/// Datatype for redeemer of the "computation stake validator" to represent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK 😄

)
}

pub fn no_datum_wrapper_validator_3(

Choose a reason for hiding this comment

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

Would it make sense to create a generalized version as well?
e.g.

pub fn no_datum_wrapper_validator_3(
  hashed_parameters: List<...>,
  parameter_serializaer: List<...>,
  ...
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that would limit the parameters to be of the same type... Unless there is a better way to model this?

pub type ParameterisedRedeemer<p, r> {
  params: List<p>,
  redeemer: r,
}

///
/// For validation functions, corresponding indices of inputs/outputs are also
/// provided in these functions.
pub fn withdraw_no_redeemer(

Choose a reason for hiding this comment

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

Can this function be split to aid readability? (Only if it does not make performance worse...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By splitting, do you mean having different variants, each of which perform one of the 3 validation functions?

/// Under the hood, one other difference is that here, instead of traversing all
/// the inputs, there are two traversals: one over the `redeemers`, and another
/// over the indices.
pub fn withdraw_with_redeemer(

Choose a reason for hiding this comment

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

Maybe split this as well? (Same caveats as above...)

/// > This function has no protection against
/// > [double satisfaction](https://github.com/Plutonomicon/plutonomicon/blob/b6906173c3f98fb5d7b40fd206f9d6fe14d0b03b/vulnerabilities.md#double-satisfaction)
/// > vulnerability, as this can be done in multiple ways depending on the
/// > contract. If you can tolerate some extra overhead, consider using the

Choose a reason for hiding this comment

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

I do not think that there is any protocol that might not want to avoid a double satisfaction attack. Maybe we could implement a countermeasure for it in this module?

Copy link
Contributor Author

@keyan-m keyan-m Nov 15, 2024

Choose a reason for hiding this comment

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

I can think of 4 ways to prevent double satisfaction:

  1. Equality of input index and output index (not applicable everywhere)
  2. Datum tagging (would require an additional function for extracting the tag field from the output datum)
  3. Filtering the inputs and expecting one to remain (a bit costly)
  4. Expecting the continuing output at index 0, such that it holds only 1 NFT (works for contracts that only hold multiple beacon UTxOs)

Picking one over the others might make the interface somewhat opinionated. What do you think?

@keyan-m
Copy link
Contributor Author

keyan-m commented Nov 15, 2024

Very nice work! Just some minor observations and questions.

Thank you for taking the time @mpetruska 🙏

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.

2 participants