Augment DelegatedDispatchVerifier trait #42
Conversation
…ct to contract dispatch verifiers
| @@ -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; | |||
There was a problem hiding this comment.
Now we have mixed associated type (AccountId) and generic type (Doughnut). It would be sensible to stick to one or the other
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Implements step 1 of #32