Sovereign-Labs / sovereign-sdk

A framework for building seamlessly scalable and interoperable rollups that can run on any blockchain
https://sovereign.xyz
Apache License 2.0
364 stars 104 forks source link

Allow dispatch_query to run directly against storage #168

Open preston-evans98 opened 1 year ago

preston-evans98 commented 1 year ago

Background

Currently, the dispatch_query method from the DispatchQuery trait in sov-modules-api takes a mutable reference to a WorkingSet as an argument. This is not very ergonomic for queriers, who have to create a new (empty) WorkingSet wrapping a storage instance and then run their queries against it.

Suggested Change

Create a new trait for queryable storage, and then have WorkingSet and ProverStorage implement it. Use this trait in DispatchQuery instead of a concrete WorkingSet

pub trait DispatchQuery {
    type Context: Context;
    type Decodable;

    /// Decode serialized query message
    fn decode_query(serialized_message: &[u8]) -> Result<Self::Decodable, std::io::Error>;

    /// Dispatches a query message to the appropriate module.
    fn dispatch_query<Q: QueryableStorage>(
        &self,
        message: Self::Decodable,
        working_set: Q,
    ) -> QueryResponse;
}
bkolad commented 1 year ago

There is also another design choice we can consider:

  1. We define query as:

    pub trait DispatchQuery {
    type Context: Context;
    type Decodable;
    
    /// Decode serialized query message
    fn decode_query(serialized_message: &[u8]) -> Result<Self::Decodable, std::io::Error>;
    
    /// Dispatches a query message to the appropriate module.
    fn dispatch_query<(
        &self,
        message: Self::Decodable,
        storage: &C::Storage,
    ) -> QueryResponse;
    }
  2. We change the way how we get data: Instead of:
    pub fn query_value(&self, working_set: &mut WorkingSet<C::Storage>) -> Response {
        Response {
            value: self.value.get(working_set),
        }
    }

    We do:

 pub fn query_value(&self, storage: &C::Storage) -> Response {
        Response {
            value: storage.get(&self.value);  <- this has changed
        }
    }

And to be consistent in call, we change:

 let admin = self.admin.get_or_err(working_set)?;

to

 let admin = working_set.get_or_err(&self.admin)?;

Advantages:

Disadvantages: Maybe

 let admin = self.admin.get_or_err(working_set)?;

is more common/intuitional than

 let admin = working_set.get_or_err(&self.admin)?;
preston-evans98 commented 1 year ago

I thought about this, but I think it will be important to be able to run queries against a working set. For example, when you run eth_call geth conceptually creates a new working set, executes against it, then runs your query against the working set and drops the set before returning.