-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A rust based Dave compute implementation #23
Conversation
What are these large json files? |
They are contract abis, I'll remove them and improve the overall binding generation process. |
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.
This is looking super nice!!
offchain/core/build.rs
Outdated
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.
Forge has the forge bind
command, which generates a Rust crate with the bindings.
I think we should create a crate just with the bindings. To do that, I think we could either extract this build.rs
logic into this "contracts bindings" crate, or use forge bind
instead. Guilherme generated Rust bindings for rollups-contracts
using forge bind
.
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.
Sure, forge bind
seems much easier to do. I was maintaining this build.rs
because Arena
was responsible for deploying the contracts and thus needed the abis. It's no longer the case that Arena
now isn't responsible for deploying the contracts.
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.
I like the way you've changed the arena/player interfaces, and the way you more explicitly split the reading and sending functionalities. I'll propose one further change, let me know what you think.
My understanding is that the Arena
object has a bunch of reading methods. These methods are called beforehand, to fetch the state. Then, we call react on Player
, passing this state. Furthermore, Arena
also implementes the ArenaSender
trait, and the Player
stores it internally.
One suggestion is to really split the Arena
object in two. One for reading states, that doesn't really implement any trait. Something like StateReader
, or ArenaReader
, or some better name. Then there's the ArenaSender
trait, which is implemented by a different object. I believe this split is already kinda there, I'm suggesting we really split them in different objects and files. I may be missing something that makes this impossible.
Also, there's another interesting design pattern in Rust which may be interesting to experiment. Currently, we have something like:
trait ArenaSender {}
struct Player<A: ArenaSender> {
arena_sender: A
}
impl<A: ArenaSender> Player<A> {
fn new(arena_sender: A) -> Self {
Self { arena_sender }
}
fn react() {}
}
Instead, we can do:
trait ArenaSender {}
struct Player {}
impl Player {
fn new() -> Self {
Self {}
}
fn react(arena_sender: impl ArenaSender) {}
// or
fn react<A: ArenaSender>(arena_sender: A) {}
}
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.
I'll certainly split the reader and the sender in different files and objects. I'm curious about what advantage you think we can benefit from the second design pattern? Is it that the Player
object is no longer bundled to a specific ArenaSender
implementation? So the Player
can freely react
using different ArenaSender
implementation (possible mocking version for testing purpose) at runtime?
2ec9570
to
b215787
Compare
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.
Looking good! I'm still reviewing.
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.
For a later PR, we could consider generating this file automatically from some configuration, but I'm not sure.
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.
It would be nice have different constants for testing. Let's think more on this later!
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.
This looks great to me! I've added a few comments that, if you agree with, we can create issues for later.
} | ||
|
||
impl TournamentState { | ||
pub fn new_root(address: Address) -> Self { |
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.
I think this method could be an implementation of std::default::Default
.
} | ||
|
||
impl StateReader { | ||
pub fn new(config: ArenaConfig) -> Result<Self, Box<dyn Error>> { |
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.
I think we can avoid Box<dyn Error>
, by either using anyhow
crate (easiest, and it's probably ok), or by generating specific error types with thiserr
.
.new_inner_tournament_filter() | ||
.address(Value(tournament_address)) | ||
.topic1(H256::from_slice(match_id.hash().slice())) | ||
.from_block(0) |
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.
We should probably change this from_block(0)
to the latest block we've queried. I suggest the following. Since there are many calls, and the only public method is pub async fn fetch_from_root
:
- creates a new field in
struct StateReader
calledlast_queried_block
. - in
fetch_from_root
, query the current latest block, and save it in a local variablelatest_block
. - pass this queried
latest_block
to all nested functions, since the latest block may change while we're querying along the chain of function calls. Change all the queriesto_block
to uselatest_block
. - change all requests to query
from
to useself.last_queried_block
. - after finishing querying the state
fetch_from_root
updatesself.last_queried_block
tolatest_block
before returning.
We might have to change &self
to &mut self
, or use a Mutex. Both makes a lot of sense, since with this change fetch_from_root
calls have to be mutually exclusive. With mutex, we'd need to acquire the lock before we do anything, and only release it once we finish everything. The &mut self
approach accomplishes the same thing at the type system level (but now we won't be able to share pointers to StateReader
). I think I prefer the &mut self
approach.
|
||
/// The [ArenaSender] trait defines the interface for the creation and management of tournaments. | ||
#[async_trait] | ||
pub trait ArenaSender: 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 want to get rid of Box<dyn Error>
, we can create an associated type:
trait T {
type Error;
async fn foo(
&self,
) -> Result<(), Self::Error>;
}
struct S {}
impl T for S {
type Error = MyConcreteError;
// ...
}
Tackle issue: #8 and #12
The implementation uses a similar structure to the lua prototype.