Skip to content
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

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Conversation

clearloop
Copy link
Contributor

@clearloop clearloop commented Jun 17, 2024

Resolves #3895

This PR basically combines gclient and gtest into the same interface which helps users re-using their code for both testing and client-side operations

The key points of the merge is that frequently used methods should be merged

- some of the methods are not synced ( like event listener, it's actually useless in tests )
- logs are playing a role part in the transaction results since it's useful for developers' centralized indexer instead of message payloads ( need to pay the rent and easy to lose )
- message builder with specified signer from wallet
  • common interfaces of debugging programs based on the design of the interface of gtest
  • gclient implementation
  • gtest implementaion refactor(gtest): make gtest thread safe #4032
  • pack the two implementation into the general Client
  • pick a test in examples and apply the Client test
  • tiny consistency test

for expected result:

#[tokio::test]
async fn test_gtest() -> Result<()> {
    test_ping(GTest::client()?).await
}

#[tokio::test]
async fn test_gclient() -> Result<()> {
    let api = GearApi::dev_from_path("../target/release/gear").await?;
    test_ping(Client::<GClient>::new(api)).await
}

fn test_ping<T: Backend>(client: Client<T>) -> Result<()> {
   // ...
}

@gear-tech/dev

@clearloop clearloop added the A1-inprogress Issue is in progress or PR draft is not ready to be reviewed label Jun 17, 2024
@clearloop clearloop added A0-pleasereview PR is ready to be reviewed by the team and removed A1-inprogress Issue is in progress or PR draft is not ready to be reviewed labels Jul 25, 2024
@clearloop clearloop marked this pull request as ready for review July 25, 2024 08:38
Copy link
Member

@DennisInSky DennisInSky left a 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/src/api/mod.rs Outdated Show resolved Hide resolved
gclient/Cargo.toml Outdated Show resolved Hide resolved
gclient/tests/client.rs Outdated Show resolved Hide resolved

#[tokio::test]
async fn test_gtest() -> Result<()> {
test_ping(GTest::client()?).await
Copy link
Member

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

Copy link
Contributor Author

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


assert!(
result
.logs
Copy link
Member

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

Copy link
Member

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

Copy link
Contributor Author

@clearloop clearloop Aug 6, 2024

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

  1. 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 the result
  2. however, the send <-> reply model is for program integrations, since the reply 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.

M: Into<Message> + Send;

/// Get mailbox message from message id
async fn message(&self, mid: MessageId) -> Result<Option<(UserStoredMessage, Interval<u32>)>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name is not obvious

Copy link
Contributor Author

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

Comment on lines 56 to 59
pub trait Code: Sized + Send {
/// Get wasm bytes
fn wasm(self) -> Result<Vec<u8>>;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

Copy link
Contributor Author

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/mod.rs Outdated Show resolved Hide resolved

async fn message(&self, _mid: MessageId) -> Result<Option<(UserStoredMessage, Interval<u32>)>> {
Err(anyhow!(
"gtest backend currently doesn't support this method"
Copy link
Member

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?

Copy link
Contributor Author

@clearloop clearloop Aug 6, 2024

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

gclient/src/client/backend/gtest.rs Outdated Show resolved Hide resolved
@clearloop
Copy link
Contributor Author

same as #4072 (comment)

@clearloop clearloop closed this Aug 22, 2024
@clearloop clearloop reopened this Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview PR is ready to be reviewed by the team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connect gclient with gtest
3 participants