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

Refactor ClientState API #584

Merged
merged 2 commits into from
Mar 1, 2022
Merged

Refactor ClientState API #584

merged 2 commits into from
Mar 1, 2022

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Feb 27, 2022

This is to address #577.
The current Client APIs are all implemented on ClientState which is per-account. This is problematic.
We want the APIs to be implemented on ClientAddressManager.
This PR moves all APIs over, and fixes all uses, tests and etc.

@lxfind lxfind marked this pull request as draft February 27, 2022 16:20
@lxfind lxfind force-pushed the client-api-refactor branch from 2bf48ab to 55c96b1 Compare March 1, 2022 07:25
@lxfind lxfind marked this pull request as ready for review March 1, 2022 07:26
@lxfind lxfind changed the title [RFC] Refactor ClientState API Refactor ClientState API Mar 1, 2022
@lxfind lxfind force-pushed the client-api-refactor branch from 55c96b1 to 12f13ed Compare March 1, 2022 07:51
@asonnino asonnino mentioned this pull request Mar 1, 2022
4 tasks
Copy link
Contributor

@patrickkuo patrickkuo left a comment

Choose a reason for hiding this comment

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

We need another round of refactoring after #594

LGTM

@@ -325,21 +325,25 @@ async fn sui_start(

// Sync all accounts.
for address in addresses.iter() {
let client_state = wallet_context
.get_or_create_client_state(address)
wallet_context
Copy link
Contributor

Choose a reason for hiding this comment

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

rest server should create its own account manager instead of using the wallet context, we can fix this after key pair removal, rest server will no longer need to handle keys after that.

Comment on lines +525 to +533
wallet_context
.create_account_state(address)
.map_err(|error| {
custom_http_error(
StatusCode::FAILED_DEPENDENCY,
format!("Could not get or create client state: {error}"),
));
}
};
let object_refs = client_state.object_refs();
)
})?;
let object_refs = wallet_context.address_manager.get_owned_objects(*address);
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't need to call create_account_state explicitly, the account manager should handle it internally, however I guess it is not doable at the moment because of the key pair, will need to refactor this after #594

@lxfind lxfind force-pushed the client-api-refactor branch from 12f13ed to 13c69dc Compare March 1, 2022 16:05
@lxfind lxfind merged commit f53fbe9 into main Mar 1, 2022
@lxfind lxfind deleted the client-api-refactor branch March 1, 2022 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants