Skip to content
This repository has been archived by the owner on Jul 4, 2022. It is now read-only.

Augment DelegatedDispatchVerifier trait #42

Merged
merged 2 commits into from
Dec 24, 2019
Merged

Augment DelegatedDispatchVerifier trait #42

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c942ee
Augment DelegatedDispatchVerifier trait so it provides runtime/contra…
Dec 23, 2019
6a9ebaa
Fix build issues and warnings
Dec 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions frame/executive/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,7 @@ mod tests {
// We aren't testing doughnut verification here just return `Ok(())`
pub struct MockDelegatedDispatchVerifier<T: system::Trait>(rstd::marker::PhantomData<T>);
impl<T: system::Trait> DelegatedDispatchVerifier<T::Doughnut> for MockDelegatedDispatchVerifier<T> {
type AccountId = T::AccountId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Now we have mixed associated type (AccountId) and generic type (Doughnut). It would be sensible to stick to one or the other

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad for missing this. It looks like we don't use this associated type anywhere. I think we should remove it to keep it consistent

Copy link
Author

@aliXsed aliXsed Jan 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We have used AccountId as the type of caller and contract addresses. DelegatedDispatchVerifier is defined generically with no knowledge about the type Doughnut or DoughnutRuntime or even generic asset.
A different solution to the one introduced here could be that we define our trait like this:
pub trait DelegatedDispatchVerifier<Doughnut, AccountId> { ... }
But this is then introducing a new generic parameter which is very similar to an associated type and worse because it makes it possible that we implement DelegatedDispatchVerifier for different combinations of Doughnut and AccountId which is not what we would need.
Introducing a dependency between frame/support and frame/system so we bring in generic asset definitions for the additional traits doesn't seem to be a good solution either.

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is that we should do all generic parameters or all associated types

pub trait DelegatedDispatchVerifier<Doughnut, AccountId> { ... }

or

pub trait DelegatedDispatchVerifier {
  type AccountId;
  type Doughnut;
}

but not a mix of both as it's confusing.

It will be good to change to all associated after #40 is resolved

const DOMAIN: &'static str = "";
fn verify_dispatch(
_doughnut: &T::Doughnut,
Expand Down
1 change: 1 addition & 0 deletions frame/executive/tests/doughnut_integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ impl_outer_dispatch! {

pub struct MockDelegatedDispatchVerifier<T: system::Trait>(rstd::marker::PhantomData<T>);
impl<T: system::Trait> DelegatedDispatchVerifier<T::Doughnut> for MockDelegatedDispatchVerifier<T> {
type AccountId = T::AccountId;
const DOMAIN: &'static str = "test";
fn verify_dispatch(
doughnut: &T::Doughnut,
Expand Down
13 changes: 13 additions & 0 deletions frame/support/src/additional_traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ impl<T, U> ChargeFee<T> for DummyChargeFee<T, U> {
/// The `verify()` hook is injected into every module/method on the runtime.
/// When a doughnut proof is included along with a transaction, `verify` will be invoked just before executing method logic.
pub trait DelegatedDispatchVerifier<Doughnut> {
type AccountId;

/// The doughnut permission domain it verifies
const DOMAIN: &'static str;
/// Check the doughnut authorizes a dispatched call to `module` and `method` for this domain
Expand All @@ -60,10 +62,21 @@ pub trait DelegatedDispatchVerifier<Doughnut> {
module: &str,
method: &str,
) -> Result<(), &'static str>;

/// Check the doughnut authorizes a dispatched call from runtime to the specified contract address for this domain.
fn verify_runtime_to_contract_dispatch(_caller: &Self::AccountId, _doughnut: &Doughnut, _contract_addr: &Self::AccountId) -> Result<(), &'static str> {
Err("Doughnut runtime to contract dispatch verification is not implemented for this domain")
}

/// Check the doughnut authorizes a dispatched call from a contract to another contract with the specified addresses for this domain.
fn verify_contract_to_contract_dispatch(_caller: &Self::AccountId, _doughnut: &Doughnut, _contract_addr: &Self::AccountId) -> Result<(), &'static str> {
Err("Doughnut contract to contract dispatch verification is not implemented for this domain")
}
}

/// A dummy implementation for when dispatch verifiaction is not needed
impl<Doughnut> DelegatedDispatchVerifier<Doughnut> for () {
type AccountId = u64;
const DOMAIN: &'static str = "";
fn verify_dispatch(_: &Doughnut, _: &str, _: &str) -> Result<(), &'static str> {
Ok(())
Expand Down
1 change: 1 addition & 0 deletions prml/doughnut/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ where
pub struct PlugDoughnutDispatcher<Runtime: DoughnutRuntime>(rstd::marker::PhantomData<Runtime>);

impl<Runtime: DoughnutRuntime> DelegatedDispatchVerifier<Runtime::Doughnut> for PlugDoughnutDispatcher<Runtime> {
type AccountId = Runtime::AccountId;
const DOMAIN: &'static str = "plug";
/// Verify a Doughnut proof authorizes method dispatch given some input parameters
fn verify_dispatch(
Expand Down