More and Better Resource Validation

[devmannic]

Continuing the discussion started in #406

@0xOmarA There are a few different related ideas. I'll try to restate much of what I said in the other PR but maybe a little more organized here with some small examples to make it more clear.

Thanks for keeping the discussion going. I'm willing to contribute some code on this to get any parts of this over the finish line for v0.5 if needed.

ProofToValidate

(aka UnValidatedProof, but I like the new name better)

Start with example code showing how it would be used

/// existing ValidatedProof usage
pub fn func1(self, proof: Proof) {
  let proof: ValidatedProof = proof.validate_proof(self.saved_resource_address).unwrap();
  assert!(proof.amount() > dec!(5));
  // ...
}

/// adding ProofToValidate without sbor::Decode implemented
pub fn func2(self, proof: Proof) {
  let proof: ProofToValidate = proof.into();
  // there are no available methods on proof now other than validate_proof so we know it must be checked before use ie:
  // assert!(proof.amount() > dec!(5)); // this wont compile
  // the rest is just like func1
  let proof: ValidatedProof = proof.validate_proof(self.saved_resource_address).unwrap();
  assert!(proof.amount() > dec!(5)); // now this is ok
  // ...
}

/// going further adding sbor::Decode (and related traits, but not necessarily Encode) to ProofToValidate
/// note ProofToValidate does not have it's own type id or description, it is still "Proof" so you cannot pass these around
/// this is just a way to enforce creation of a ProofToValidate when the function call is dispatched.  (Just like in scrypto_statictypes)
pub fn func3(self, proof: ProofToValidate) {
  // let proof: ProofToValidate = proof.into();  // this line is no longer needed since proof.into() happens implicitly during decode at method invocation time.
  //
  // the rest is just like func2
  let proof: ValidatedProof = proof.validate_proof(self.saved_resource_address).unwrap();
  assert!(proof.amount() > dec!(5)); // now this is ok
  // ...
}

The goal here is for developers to have a way to indicate in the function signature (or shortly after) that they intent to validate the Proof, and the implementation guarantees there will be no code paths where the proof contents (address, amount, ids, data, etc) can be accessed prior to the validation.

Then ProofToValidate can be extended with more complex validation functionality in addition to checking just the resource. And this is separate from the "low level" Proof.

NOTE It's worth saying that I think after having ProofToValidate you might consider completely removing ValidatedProof and simply have ProofToValidate transition back to a Proof. Why keep around the validated wrapper type if you know validation must have happened upfront and is complete? Makes for lots less code to maintain or one less type for devs to understand. The biggest downside to this removal is it makes implementing Drop much harder. Se the next section.

impl Drop for ValidatedProof and also maybe Deref

Since ValidatedProof is a simple wrapper it's a good time to implement the Drop trait so developers can stop worrying about manually calling .drop() and all the weird issues that crop up in complex code. Rust can manage all this, we should let it. I already have an implementation that would work with almost no changes in my ProofOf type in scrypto_statictypes. Do note that it also makes use of the Deref trait to forward method calls to the wrapped Proof instead of reimplementing all the sfunctions! stuff manually. I like my approach there better since it will always be in sync with Proof without future code changes and keeping things in sync. But since fees are coming I don't know exactly what the full tradeoff is. As it is it looks like @0xOmarA may have missed some functions in PR #406 that are part of Proof, but now not exposed by ValidatedProof.

You can see my existing implementation here: https://github.com/devmannic/scrypto_statictypes/blob/89472d7e2f6b5dc585981c8b78739a6dfcb22430/src/proofof.rs#L97

It requires the wrapper to have a RefCell<Option<Proof>> instead of just a Proof and thus a check in every method call as a result. There is a performance (and thus a fee) hit for this type of implementation but I can't quantify it yet.

More Validation

One approach I could see is a ResourceValidation trait (or family of traits, probably with generics) created which can be implemented for ProofToValidate and maybe there's also a BucketToValidate that works similarly, but also implement these for Proof and for Bucket (and for Vault), all so that devs can more declaratively indicate requirements (aka constraints) and check them correctly. These would provide consistancy with all the same methods and probably with very similar or even common implementation with macros or generics. Having devs continuing to implementing comparisons against amount() or specific tests against addresses, or nonfungible ids, really just seems like grunt work. They should be able to declare their constraints and be assured that they are checked properly as much as possible.

What that trait's methods look like is still worth discussing but I really liked my HareSwap BucketRequirement implementation here:https://github.com/devmannic/scrypto-challenges/blob/main/1-exchanges/hareswap/src/requirement.rs
It needs to be updated for scrypto > 0.3 but it's mostly just renaming. The idea being that a dev would instantiate a ResourceRequirement and then pass that instance into ProofToValidate::validate_proof.

To that end we would then make validate_proof part of these traits (maybe a more specific name instead).

Then for more complex validation, you simply add more to the trait, and implement it. If the fees need to be subsidized or for some reason it's better to do the implementation on the RE side, then do it there, otherwise do it on the "user" side. Our discussion about the iterator gets to this point. You can add a method which takes an iterator. It's a nice approach, but it's also less efficient to check since it require lots of wasm boundary crossings since the iterator is by design lazy evaluated. Instead (or in addition) providing a method which takes a set of ResourceAddresses probably the way to go. Then that can be a single RE call and have whatever fee schedule you want.

Example

pub fn func4(self, proof: ProofToValidate) {
  let requirement = ProofRequirement::contains_at_least(self.saved_resource_address, dec!(5)); // at least 5 of the resource
  let proof: ValidatedProof = proof.validate_proof_against(requirement).unwrap();
  // assert!(proof.amount() > dec!(5)); // // this is part of the requirement check not needed here
  // ...
}

AccessRule-style validation

Taking this further or maybe in a slightly different direction, another option: I think having some parallels to the AccessRule's design would be really really nice, but that might be harder to develop, I"m not sure. But in return you get a single common interface to talk about the contents of Proofs (and Buckets, and Vaults) for checking access, or for checking for business logic which I just think makes a lot of sense. Less for devs to learn, more consistency so easier to build and audit, etc.

This might look something like:

/// exact same macros and similar API as AccessRules.  Supports same logic it already has, and all the checks can happen in the RE and fees set as needed
pub fn func5(self, proof: ProofToValidate, bucket: Bucket) {
  let requirement = ResourceRules::new(rule!(require_n_of(dec!(5), self.saved_resource_address))); // at least 5 of the resource
  let proof: ValidatedProof = proof.validate_proof_against(requirement).unwrap();
  // assert!(proof.amount() > dec!(5)); // this is part of the rules check not needed here
  // ...
  // then use the same requirement for something else with or without "validate" and wrapper types.
  // this example just on a plain Bucket.
  if bucket.check_rules(requirement) {
    // ...
  }
}

no more "unsafe"

I'll say it again. I don't like unsafe_skip_proof_validation.

It serves almost no purpose, even now before more advanced validation is implemented. If a dev wants to or needs to do their own validation anyway, then they get no value from using a ValidatedProof. It doesn't have any methods a Proof doesn't have. It really only adds semantic confusion. The "unsafe" method literally only exists to potentially signpost that a complex check might be done, and make it easier to not validate things. Wouldn't it be better to just let the complex code exist, and have the lack of a ValidatedProof actually be a better indication the code is not safe because you cannot actually ensure it's been validated?

In any case, even if we disagree here, the better answer is to provide more advanced validation options for devs and for sure after that remove the "unsafe" method as it really wont be needed.

Love to hear any thoughts anyone has on these ideas. Thanks.

[0xOmarA]

Hey @devmannic, thank you for starting this Github issue and for continuing the discussion on ValidatedProofs.

The goal here is for developers to have a way to indicate in the function signature (or shortly after) that they intent to validate the Proof, and the implementation guarantees there will be no code paths where the proof contents (address, amount, ids, data, etc) can be accessed prior to the validation.

So, in your proposal, would somebody who does not know the resource address of a Proof be able to access the data on the proof in any kind of way?

I can see both sides of the argument in this case, on one hand, a Proof is only ever a Proof when you know what to expect. When I say that I will show you proof of me owning a car, you expect a proof of car ownership, and not any random proof. So, I get the argument that the other side always knows what to expect. On the other hand, completely disallowing any kind of way of getting data on a Proof unless its validated seems like it would be too restrictive and might break some dApps or make them harder to work with.

The one example that comes to mind where not being able to access the proof data becomes a problem is the following: say that you have some kind of dApp which allows people to register. The dApp has two methods for registration:

/// Registers a user and returns to them a user badge and their change
pub fn register(&mut self, payment: Bucket) -> (Bucket, Bucket);

/// Allows the user to register using a badge that they own (to minimize transaction fees)
pub fn register_with_owned_badge(&mut self, payment: Bucket, badge: Proof) -> Bucket;

In the above code example, the register_with_owned_badge breaks if there is no way of getting the resource address of the Proof. Perhaps one work around would be to change the method signature to:

pub fn register_with_owned_badge(&mut self, payment: Bucket, badge: Proof, badge_resource_address: ResourceAddress) -> Bucket;

But the above feels weird to me for two reasons:

1. In an almost-physical way, I have a Proof of the badge with me, why should the caller give me the ResourceAddress of that badge for verification?
2. I fear that this might have an impact on composability.

To be clear, I am not against there not being any code paths to get the content of a proof prior to validation, it is just that this is a big change and I'm trying to see if there are any use-cases that break if this change is made, and if they do, how can we support them or enable them.

Then ProofToValidate can be extended with more complex validation functionality in addition to checking just the resource. And this is separate from the "low level" Proof.

I have thought about this a little bit more and I quite like it, perhaps the proof validation could be expressed through something like:

enum ProofValidationCriteria {
    /// Validates that the passed proof is of the given resource address
    ResourceAddress(ResourceAddress),

    /// Validates that the passed proof is of any of the given resource addresses
    AnyOfResourceAddress(BTreeSet<ResourceAddress>),

    /// Validates that the proof contains the specified amount
    Amount(ResourceAddress, Decimal),
    
    /// Validates that the proof contains at least the specified amount
    AtLeast(ResourceAddress, Decimal),
    
    /// Validates that the proof contains at most the specified amount
    AtMost(ResourceAddress, Decimal),

    /// Validates that the proof contains a Non Fungible with a given Id
    NonFungible(ResourceAddress, NonFungibleId),
    
    /// Validates that the proof contains a Non Fungible in a given set of Ids
    NonFungible(ResourceAddress, BTreeSet<ResourceAddress>),
    
    /// Validates that the proof contains a Non Fungible in a given set of Ids
    AnyOfNonFungibles(BTreeSet<NonFungibleAddress>),
}

impl From<ResourceAddress> for ProofValidationCriteria {
    ...
}
// Other useful From traits go here too

impl Proof {
    pub fn validate_proof(self, validation_criteria: ProofValidationCriteria) -> Result<ValidatedProof>
}

What do you think? Another enum variant which could be added is AccessRule but thats a bit of an overkill since AccessRules are meant to be checked over an array of proofs and not a single proof.

Since ValidatedProof is a simple wrapper it's a good time to implement the Drop trait so developers can stop worrying about manually calling .drop() and all the weird issues that crop up in complex code. Rust can manage all this, we should let it. I already have an implementation that would work with almost no changes in my ProofOf type in scrypto_statictypes.

I can't immediately comment on that and will have to check out your implementation of it first.

As it is it looks like @0xOmarA may have missed some functions in PR #406 that are part of Proof, but now not exposed by ValidatedProof.

Can you please elaborate a little bit more on what was missed?

Taking this further or maybe in a slightly different direction, another option: I think having some parallels to the AccessRule's design would be really really nice, but that might be harder to develop, I"m not sure. But in return you get a single common interface to talk about the contents of Proofs (and Buckets, and Vaults) for checking access, or for checking for business logic which I just think makes a lot of sense. Less for devs to learn, more consistency so easier to build and audit, etc.

The only problem I have with this is that there is no way to enforce, at compile-time, that a rule can be realistically fulfilled. As an example, I will use the code that I provided as an example:

pub fn func5(self, proof: ProofToValidate, bucket: Bucket) {
    let requirement = ResourceRules::new(rule!(require(admin_badge) && require(super_admin_badge) )); 
    let proof: ValidatedProof = proof.validate_proof_against(requirement).unwrap();
    if bucket.check_rules(requirement) {
      // ...
    }
  }

The above code is syntactically correct, but logically incorrect as a Proof can't contain an admin badge and a super admin badge at the the same time. The only instance (in my personal opinion) where an AccessRule is useful is when we have an array or vector or proofs. Aside from that, I personally prefer an Enum which provides compile-time guarantees that the resource rules are realistic.

If we wanted to do this, we could most likely make the ResourceRules::new() function take in a ProofRule instead of an AccessRule.

[devmannic]

Well @0xOmarA I just realized I totally misread a big part of your PR. The inline diff did not make it clear to me but I see now while I'm comparing the entire new file that you have in fact removed a bunch of methods from Proof and instead put them on ValidatedProof. Is that right? I couldn't tell that's what happened looking at the non side-by-side diff... my mistake. And so that's not what I had envisioned and much of my discussion around the need for ProofToValidate assumes that Proof still exists as it always did.

So, I'm mostly ok with doing it the way you've done it but it changes a lot of what I've been saying related to the needs for ProofToValidate and it does bring about your exact questions related to "what if the dev can't validate it"). Also, it makes your changes massively not backwards compatible for everybody. If the mindset is now that a Proof is basically unusable until it's validated, that is the same overall goal I was going for with ProofToValidate just arrived at in a different way. The problem is your way basically requires an unsafe trapdoor and gives no way to do complex validation (exactly the issues you initially described). My way doesn't have those problems.

So, I think I still like my design better where Proof stays a Proof and all the internal RE and existing code that wants to use a Proof (in a way that is more than just moving around the id) still just uses it, but when a developer wants to enforce validation they use ProofToValidate. This seems like a much more reasonable approach to me. This allows you to separate the "easy" developer-facing "upper" API from the internal "lower" API more readily.

So, in your proposal, would somebody who does not know the resource address of a Proof be able to access the data on the proof in any kind of way?
...
To be clear, I am not against there not being any code paths to get the content of a proof prior to validation, it is just that this is a big change and I'm trying to see if there are any use-cases that break if this change is made, and if they do, how can we support them or enable them.

EDIT: ironically, I think your changes already are actually a much bigger change than I was proposing or envisioning as it requires everyone to move to ValidatedProof. Read the next paragraph as if Proof is unchanged as that was my intent when I wrote it before I realized the full extent of the changes in your PR.

I think you're making this out to be a bigger change then it really is, and maybe our assumptions are not lined up. Your questions seem to imply that the change I'm suggestion somehow globally replaces Proof with ProofToValidate and makes Proof no longer usable. That's not the point at all. The goal is that when a developer expects to validate a proof they use ProofToValidate. That means they obviously must be able to get a resource address (or set of) at a minimum because that's the minimum information to perform validation. If the developer doesn't have that, then they just don't use ProofToValidate because there's no point to using it if they can't actually validate a proof. They simply use Proof instead, as they always have, with immediate access to the underlying attributes and data. Since they are the one writing the function/method which takes an argument, they are free to choose the type they need that matches their requirements. Again, unsafe trapdoors are not needed because if they dont want to validate the proof they just use Proof instead as they always have. (Further that makes these changes all 100% backwards compatible). That is in effect the only trapdoor, to not use ProofToValidate at all. And do take note that because my suggested implementation is to have ProofToValidate decode directly from Proof (and idealy Encode to a Proof as well) then the use of this never affects the ABI, it only changes how the function/method implementer chooses to handle the Proof they've received. I think this is important for handling versioning of blueprints down the line.

EDIT: So again, I think we've basically arrived at the same place with this two different approaches, but mine is backwards compatible, and does not require "unsafe" trapdoor methods, and provides a separation for low-level (or even in RE) and high-level interactions with Proofs.

---

What do you think? Another enum variant which could be added is AccessRule but thats a bit of an overkill since AccessRules are meant to be checked over an array of proofs and not a single proof.

EDIT: again, read this as if Proof exists unchanged

I like the ProofValidationCriteria enum. It's better than having many individual validtion methods for the "easy" cases, instead having different enum variants. Though in your implementation example, instead of impl Proof {...} I would suggest making a ResourceValidation trait with a method that returns bool, and then impl ResourceValidation for ... for Proof, Bucket, Vault, ProofToValidate. Finally, add validate method to that trait (or to another trait which requires ResourceValidation) which always returns a ValidatedProof which can then be applied to both ProofandProofToValidate. Additionally, it might be nice to consider this method as another way to get a proof (in this case a ValidatedProof) from a BucketorVault`. Then it'd be a common api for everything.

---

Can you please elaborate a little bit more on what was missed?

OH... and this is where I realize inline diffs sucks. It looked lik you hadn't added things like "contains" to ValidatedProof when in fact you moved them all from Proof... never mind...

---

If we wanted to do this, we could most likely make the ResourceRules::new() function take in a ProofRule instead of an AccessRule.

Yes, that's exactly what i meant. Along the same lines of rule! but not exactly an AccessRule. For exactly the reason you mentioned, "&&" doesn't always make sense and it's not an AccessRule anyway logically, it's not for access. I meant the interface using the macro that joins require... things would then have a matching API to AccessRule so it would look the same to developers, that's the benefit I was seeing with this setup, not that they would reuse AccessRule directly. So yes use ProofRule but I think more new code is needed to build up a tree of ProofRules in the same way, but I haven't looked at that in a while. In any case, that's the idea. Consistency in form and function compared to AccessRule but not reusing the same type.