-
Notifications
You must be signed in to change notification settings - Fork 106
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
feat(gclient): introduce gear general client #4016
base: master
Are you sure you want to change the base?
Conversation
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.
Good job!
InI would suggest checking Sails stuff. Maybe you will find some ideas for improvement. Also, I would suggest moving out functionality for this common client
(better name is required) into a new crate with 2 feature flags: gtest
, gclient
which will enable corresponding impls for backend. Plus making this crate re-exporting both gtest
and gclient
. If one wants to use this common client they will be using it instead of using gclient
and using gtest
separately (gclient imports the latter, but doesn't re-export it)
gclient/tests/client.rs
Outdated
|
||
#[tokio::test] | ||
async fn test_gtest() -> Result<()> { | ||
test_ping(GTest::client()?).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.
From the Sails experience I would suggest passing some actor id into the client call. It plays pretty much the same role as the signer, i.e. suri, in the case of using gclient
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.
pairs are managed by Client::add_pair and Message::signer
gclient/tests/client.rs
Outdated
|
||
assert!( | ||
result | ||
.logs |
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.
Generally speaking result.log
way to far from being a convenient abstraction. It would be better if there is a straight option to get response as a payload, error or whatever + there is an option to sbscribe to events. All that is implemented in Sails and @vobradovich can provide more details on that
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 it's better to expose events as a Stream (in Backand trait). We did it in sails for gclient & gtest
https://github.com/gear-tech/sails/blob/master/rs/src/gsdk/calls.rs#L125
https://github.com/gear-tech/sails/blob/master/rs/src/gtest/calls.rs#L171
For gtest we collect all events from RunResult and send them over the channel
https://github.com/gear-tech/sails/blob/master/rs/src/gtest/calls.rs#L89
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.
Generally speaking result.log way to far from being a convenient abstraction. It would be better if there is a straight option to get response as a payload, error or whatever + there is an option to sbscribe to events.
logs are more frequently used than response in production
- for a DEX example, user send request to swap 5 USD for 1 DOT, logging the result
Alice swapped 1 DOT with 5 USD
is cheaper than sending the user a message with theresult
- however, the
send <-> reply
model is for program integrations, since thereply
need to be paid for staying on chain, it will be logged as well for saving money in production
I think it's better to expose events as a Stream (in Backand trait).
if I'm not mistaken, this is for unifying the subscribe
method of gtest & gclient, imo we don't need to implement the subscribe / listen
method for gtest
since it is not for tests )))
developers can just verify if their logic works as expected and checking the logs of each calls, otherwise they need to use the gclient Client
for setting up their integration tests, testnet, etc.
gclient/src/client/backend/mod.rs
Outdated
M: Into<Message> + Send; | ||
|
||
/// Get mailbox message from message id | ||
async fn message(&self, mid: MessageId) -> Result<Option<(UserStoredMessage, Interval<u32>)>>; |
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.
Name is not obvious
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.
there are only few methods in this Backend
trait, complex messages are embedded in the Message builders, so send
for send message
, message
for get message
could be neat
gclient/src/client/backend/mod.rs
Outdated
pub trait Code: Sized + Send { | ||
/// Get wasm bytes | ||
fn wasm(self) -> Result<Vec<u8>>; | ||
} |
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.
pub trait Code: Sized + Send { | |
/// Get wasm bytes | |
fn wasm(self) -> Result<Vec<u8>>; | |
} | |
pub trait WasmCode: Sized + Send { | |
/// Get wasm bytes | |
fn bytes(self) -> Result<Vec<u8>>; | |
} |
Also what's the reason behiind of consuming 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.
removed self, we don't have other Code
excepct WasmCode
so Code
could be enough, Code::wasm
points to the wasm
format ( different from wat
)
gclient/src/client/backend/gtest.rs
Outdated
|
||
async fn message(&self, _mid: MessageId) -> Result<Option<(UserStoredMessage, Interval<u32>)>> { | ||
Err(anyhow!( | ||
"gtest backend currently doesn't support this method" |
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.
Should it not be a part of the trait for now and be just a pub fn of the impl?
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.
ideally, there will be just a Client
exported in the mode instead of {Client, GTest, GClient}
, so pub method for GClient
is not considered since it will make GClient
back to GearApi
xd
since gclient and gtest have different targets, some methods of Backend
are only supported in one of them, similarly, the subscribe
method will only work in the gclient client
same as #4072 (comment) |
Resolves #3895
This PR basically combines
gclient
andgtest
into the same interface which helps users re-using their code for both testing and client-side operationsClient
examples
and apply theClient
testfor expected result:
@gear-tech/dev