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 1 commit
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
Prev Previous commit
small ok_or_else refactor
  • Loading branch information
@arun-koshy
arun-koshy committed Feb 25, 2022
commit d002c0220516c93bd40e5a76c6d5537bb0259773
9 changes: 4 additions & 5 deletions sui/src/rest_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,14 +346,13 @@ async fn get_addresses(
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(
let mut wallet_context = wallet_context.ok_or_else(|| {
HttpError::for_client_error(
None,
StatusCode::FAILED_DEPENDENCY,
"Wallet Context does not exist.".to_string(),
));
}
let mut wallet_context = wallet_context.unwrap();
)
})?;

let addresses: Vec<SuiAddress> = wallet_context
.address_manager
Expand Down