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.

Sign up for GitHub

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

Add endpoint to get managed addresses #556

Merged
merged 4 commits into from
Feb 25, 2022
Merged

Add endpoint to get managed addresses #556

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f627a5
Add get addresses endpoint
arun-koshy Feb 24, 2022
9423477
small error output change
arun-koshy Feb 24, 2022
a1e335f
add todos and correctly store wallet context on early exit
arun-koshy Feb 25, 2022
d002c02
small ok_or_else refactor
arun-koshy Feb 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 76 additions & 15 deletions sui/src/rest_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ use serde_json::json;
use sui::config::{Config, GenesisConfig, NetworkConfig, WalletConfig};
use sui::sui_commands;
use sui::wallet_commands::WalletContext;
use sui_core::authority_client::AuthorityClient;
use sui_core::client::{Client, ClientState};
use sui_core::client::Client;
use sui_types::base_types::*;
use sui_types::committee::Committee;

Expand Down Expand Up @@ -48,6 +47,7 @@ async fn main() -> Result<(), String> {
api.register(start).unwrap();
api.register(genesis).unwrap();
api.register(stop).unwrap();
api.register(get_addresses).unwrap();

let api_context = ServerContext::new();

Expand Down Expand Up @@ -283,9 +283,13 @@ async fn start(
format!("Can't create client state: {error}"),
)
})?;
if let Err(err) = sync_client_state(client_state).await {
return Err(err);
}

client_state.sync_client_state().await.map_err(|err| {
custom_http_error(
StatusCode::FAILED_DEPENDENCY,
format!("Sync error: {:?}", err),
)
})?;
}

*server_context.wallet_context.lock().unwrap() = Some(wallet_context);
Expand Down Expand Up @@ -321,17 +325,74 @@ async fn stop(
Ok(HttpResponseUpdatedNoContent())
}

async fn sync_client_state(
client_state: &mut ClientState<AuthorityClient>,
) -> Result<(), HttpError> {
// synchronize with authorities
let res = async move { client_state.sync_client_state().await };
res.await.map_err(|err| {
custom_http_error(
/**
* `GetAddressResponse` represents the list of managed accounts for this client.
*/
#[derive(Deserialize, Serialize, JsonSchema)]
struct GetAddressResponse {
addresses: Vec<String>,
}

/**
* [WALLET] Retrieve all managed accounts.
*/
#[endpoint {
method = GET,
path = "/wallet/addresses",
}]
async fn get_addresses(
rqctx: Arc<RequestContext<ServerContext>>,
) -> Result<HttpResponseOk<GetAddressResponse>, HttpError> {
let server_context = rqctx.context();
// TODO: Find a better way to utilize wallet context here that does not require 'take()'
let wallet_context = server_context.wallet_context.lock().unwrap().take();
Copy link
Contributor

Choose a reason for hiding this comment

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

The good thing is after this line you now own the contents of wallet_context. The bad thing is you just replaced the original by None, so if you do an early return anywhere before you hit the line where you place the wallet_context back where you found it:

    *server_context.wallet_context.lock().unwrap() = Some(wallet_context);

then any further actions don't have a wallet_context any more!

This doesn't matter for the very next error (conditioned on wallet_context.is_none()), but the two following errors (out of get_or_create_client_state and sync_client_state) are a concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran into just this issue yesterday in some other endpoints! I have been trying to find a solution where I don't have to add that line to every endpoints early exit scenario (which gets really repetitive in future endpoints). However have yet to find anything outside of take() that doesn't cause the function change we saw earlier.

I will add the line to put wallet context back on an early exit and add a todo to find a better solution for this.

if wallet_context.is_none() {
return Err(HttpError::for_client_error(
None,
StatusCode::FAILED_DEPENDENCY,
format!("Sync error: {:?}", err),
)
})
"Wallet Context does not exist.".to_string(),
));
}
let mut wallet_context = wallet_context.unwrap();

let addresses: Vec<SuiAddress> = wallet_context
.address_manager
.get_managed_address_states()
.keys()
.copied()
.collect();

// TODO: Speed up sync operations by kicking them off concurrently.
// Also need to investigate if this should be an automatic sync or manually triggered.
for address in addresses.iter() {
let client_state = match wallet_context.get_or_create_client_state(address) {
Ok(client_state) => client_state,
Err(err) => {
*server_context.wallet_context.lock().unwrap() = Some(wallet_context);
return Err(custom_http_error(
StatusCode::FAILED_DEPENDENCY,
format!("Can't create client state: {err}"),
));
}
};

if let Err(err) = client_state.sync_client_state().await {
*server_context.wallet_context.lock().unwrap() = Some(wallet_context);
return Err(custom_http_error(
StatusCode::FAILED_DEPENDENCY,
format!("Can't create client state: {err}"),
));
}
}

*server_context.wallet_context.lock().unwrap() = Some(wallet_context);

Ok(HttpResponseOk(GetAddressResponse {
addresses: addresses
.into_iter()
.map(|address| format!("{}", address))
.collect(),
}))
}

fn custom_http_error(status_code: http::StatusCode, message: String) -> HttpError {
Expand Down