-
Notifications
You must be signed in to change notification settings - Fork 795
Conversation
ReceiverValueCap(HashMap<Address, U256>), | ||
InvalidSelector(HashSet<Selector>), | ||
InvalidReceiverSelector(HashMap<Address, Selector>), | ||
// Other(Box<dyn Fn(&TypedTransaction) -> Result<(), PolicyError> + Send + Sync>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you figure out a way to get this working, it's worth turning ensure
to async
, and making the Other variant be an async function. That way, we could even contact 3rd party service providers (or policy engines), and even read any expensive rules (e.g. ones which may need a lot of policy info from disk) concurrently. h/t @prestwich for this suggestion
Err(()) | ||
} | ||
} | ||
|
||
/// Middleware used to enforce certain policies for transactions. | ||
#[derive(Clone, Debug)] | ||
pub struct PolicyMiddleware<M, P> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need a policy combinator, otherwise all PolicyMiddleware
s in the middleware stack will be evaluated sequentially
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type TypelessError = Box<dyn Error + Send + Sync>;
type TypelessPolicy = Box<dyn Policy<Error = TypelessError>>;
#[derive(Debug)]
struct PolicyCombinator {
inner: Vec<TypelessPolicy>,
}
#[async_trait]
impl Policy for PolicyCombinator {
type Error = TypelessError;
async fn ensure_can_send(&self, tx: TypedTransaction) -> Result<TypedTransaction, Self::Error> {
let futs = self.inner.iter().map(|i| i.ensure_can_send(tx.clone()));
// assumes that no policy modifies the tx request
Ok(join_all(futs)
.await
.into_iter()
.collect::<Result<Vec<_>, _>>()?
.into_iter()
.nth(0)
.unwrap())
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question:
is there a way to structure the Policy
trait that does not require type-erasing the error in order to build a combinator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made it easier to type erase things in the meantime
/// A buildable policy combinator
#[derive(Debug)]
pub struct PolicyCombinator {
inner: Vec<TypelessPolicy>,
}
#[async_trait]
impl Policy for PolicyCombinator {
type Error = TypelessError;
async fn ensure_can_send(&self, tx: TypedTransaction) -> Result<TypedTransaction, Self::Error> {
let futs = self.inner.iter().map(|i| i.ensure_can_send(tx.clone()));
// assumes that no policy modifies the tx request
Ok(join_all(futs)
.await
.into_iter()
.collect::<Result<Vec<_>, _>>()?
.into_iter()
.nth(0)
.unwrap_or(tx))
}
}
impl PolicyCombinator {
/// Add a policy to this combinator
pub fn with<P, E>(mut self, policy: P) -> Self
where
P: Policy<Error = E> + 'static,
E: std::error::Error + Send + Sync + 'static,
{
let policy = Erased { inner: policy };
self.inner.push(Box::new(policy));
self
}
/// Converts into a PolicyMiddleware
pub fn wrap<M>(self, middleware: M) -> PolicyMiddleware<M, PolicyCombinator>
where
M: Middleware,
{
PolicyMiddleware::new(middleware, self)
}
}
#[derive(Debug)]
struct Erased<P, E>
where
P: Policy<Error = E>,
E: std::error::Error + Send + Sync + 'static,
{
inner: P,
}
#[async_trait]
impl<P, E> Policy for Erased<P, E>
where
P: Policy<Error = E>,
E: std::error::Error + Send + Sync + 'static,
{
type Error = TypelessError;
async fn ensure_can_send(&self, tx: TypedTransaction) -> Result<TypedTransaction, Self::Error> {
Ok(self.inner.ensure_can_send(tx).await?)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool,
just to make sure I understand that right, this is aimed to make it easier to combine different Policy
implementations, such as MemoryPolicy
which is treated as a combination of several Rules
which represent cases you might want to check against. Since they don't require async, I'd stick to the current implementation?
The PolicyCombinator looks like a great way to then combine custom Policy impls, for example combine multiple database lookups?
looking at this
let futs = self.inner.iter().map(|i| i.ensure_can_send(tx.clone()));
// assumes that no policy modifies the tx request
Ok(join_all(futs)
.await
.into_iter()
.collect::<Result<Vec<_>, _>>()?
.into_iter()
.nth(0)
.unwrap_or(tx))
I'm thinking the ensure_can_send
needs some work, maybe we make this take an Arc<Tx>
to ensure it wont get modified and to remove the need for cloning the tx for each Policy
The policy middleware will then be something like:
tx = Arc::new(tx);
select first failing policy(tx.clone())?;
inner(tx.unwrap_or_clone)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about a COW?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also heres a version that returns all errors instead of only 1
let (oks, errs): (_, Vec<_>) = join_all(futs).await.into_iter().partition(|x| x.is_ok());
if errs.is_empty() {
Ok(oks.into_iter().nth(0).unwrap_or(Ok(tx)).unwrap())
} else {
Err(errs.into_iter().map(|r| r.unwrap_err()).collect())
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also collect::<Result<_, _>, _>?
and this will return on the first error occurred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh this is in case you specifically want to return ALL errors in a Result<_, Vec<Error>>
It'd be nice if instead of filtering on |
I think given we haven't thought about this for some time, and in the interest of keeping the repo PRs clean, we can close this and revisit if we want it again @mattsse? |
Motivation
Follow up #400 to add an in-memory policy implementation
Solution
very primitive rule set as enum which variants represent individual rules.
There is still some work to be done, any feedback is welcome.