[wallet + rest server] Remove usage of wallet context in rest server #817

patrickkuo · 2022-03-13T22:34:59Z

Refactored Config, the configs used in wallet and rest server are now in memory only by default, PersistedConfig is added if we need to persist the config to disk.
Remove use of wallet context and wallet config from rest server
refactored rest server's ServerContext, reduced number of ArcMutex objects
Removed default implementation of wallet and network config, as it's confusing and could lead to wrong config being used.

patrickkuo · 2022-03-14T18:13:34Z

I have tried all endpoint manually and they seems to work fine, @arun-koshy can you verify it for me?

velvia

So apologies if I'm stirring the pot too much, but I have a set of overriding questions.

Instead of using PersistedConfig, have we thought of using a config library such as config-rs which can deserialize from multiple formats into structs? It also has many features we might want later, like ability to merge multiple configs or from the environment.
JSON is kinda hard to read as a config format, have we thought about using TOML?
Since you are refactoring some things could we rename "network.conf" to "sui.conf" so it can encompass things like logging/tracing config? We'll want to add much more configuration than just networking soon.

Thanks for including me, this is very interesting.

velvia · 2022-03-14T18:31:21Z

sui/src/config.rs

    }
+}
+
+pub struct PersistedConfig<C> {


Can we add RustDoc here? Not clear what PersistedConfig is for just by looking here

patrickkuo · 2022-03-14T20:20:01Z

So apologies if I'm stirring the pot too much, but I have a set of overriding questions.

Thanks for the review @velvia ! Any form of stirring are welcome here :)

Instead of using PersistedConfig, have we thought of using a config library such as config-rs which can deserialize from multiple formats into structs? It also has many features we might want later, like ability to merge multiple configs or from the environment.

PersistedConfig is a simple wrapper to add persistent to Config, we used to assume all config are persisted but it is causing inconvenience in rest server as it is using the network.conf transiently (the configs are deleted in the stop endpoint)
This splits out the persistent so the rest server is not forced to store configs to disk.

config-rs looks interesting, we can definitely explore adopting it if we need the extra functionalities.

JSON is kinda hard to read as a config format, have we thought about using TOML?

Personally I am ok with JSON/TOML/YAML. We should wait after GDC if we want to change it? There are lot of docs base on the JSON version already, not sure if we have enough time to change it?

Since you are refactoring some things could we rename "network.conf" to "sui.conf" so it can encompass things like logging/tracing config? We'll want to add much more configuration than just networking soon.

I am not so sure about extending network.conf...
network.conf is meant for local demo network, it is storing multiple authorities' key pair in the file for stating a few instances of authorities locally, I think it's not suitable to use in a real network.

I would imagine in a real Sui network, we will have one authority.conf for each of the authority, and one gateway.conf for each instance of the gateway; network.conf, sui genesis and sui start should retire after we have testnet.

arun-koshy · 2022-03-14T22:29:00Z

sui/src/rest_server.rs

+    // and we cannot use &mut reference on the server_state object.
+    fn take_server_state(&self) -> Result<ServerState, HttpError> {
+        let mut state = self.server_state.lock().unwrap();
+        state.take().ok_or_else(|| {


Could you also add this issue to the todo, I believe the take() I had in the code is what is causing the problem

velvia · 2022-03-14T22:29:57Z

PersistedConfig is a simple wrapper to add persistent to Config, we used to assume all config are persisted but it is causing inconvenience in rest server as it is using the network.conf transiently (the configs are deleted in the stop endpoint) This splits out the persistent so the rest server is not forced to store configs to disk.

config-rs looks interesting, we can definitely explore adopting it if we need the extra functionalities.

Ok cool, we can add it later, agreed. Just didn't want to have to reinvent anything, but it looks small for now.

Personally I am ok with JSON/TOML/YAML. We should wait after GDC if we want to change it? There are lot of docs base on the JSON version already, not sure if we have enough time to change it?

Oh probably not now, but can be easily changed later. We can wait for feedback from GDC/testing.

Since you are refactoring some things could we rename "network.conf" to "sui.conf" so it can encompass things like logging/tracing config? We'll want to add much more configuration than just networking soon.

I am not so sure about extending network.conf... network.conf is meant for local demo network, it is storing multiple authorities' key pair in the file for stating a few instances of authorities locally, I think it's not suitable to use in a real network.

I would imagine in a real Sui network, we will have one authority.conf for each of the authority, and one gateway.conf for each instance of the gateway; network.conf, sui genesis and sui start should retire after we have testnet.

Ah I c. In that case I might create a separate authority.conf for a separate change.

666lcz · 2022-03-15T07:12:05Z

sui/src/rest_server.rs

    // Need to use a random id because rocksdb locks on current process which
    // means even if the directory is deleted the lock will remain causing an
    // IO Error when a restart is attempted.


we might need to update the location of this comment

666lcz · 2022-03-15T07:34:25Z

sui/src/sui_commands.rs

        };
+
+        addresses.push(address);


Could you help me understand the change here? Previously we do not include the address in genesis_conf

we don't include address in genesis.conf previously because we don't have the key pair, we can add them now as we have decouple keys from the ClientState.

666lcz · 2022-03-15T07:42:52Z

sui/src/rest_server.rs


    let object_id = match ObjectID::try_from(object_info_params.object_id) {
        Ok(object_id) => object_id,
        Err(error) => {
-            *server_context.wallet_context.lock().unwrap() = Some(wallet_context);
            return Err(custom_http_error(


Why don't we have server_context.set_server_state(state); here?

The fact that we need to add server_context.set_server_state(state); before every return seems very error prone to me. I would suggest put everything in the middle into a separate function like #708 (comment) so that you only need to do it once

let state = server_context.take_server_state()?; object_schema_helper(state,...); server_context.set_server_state(state);

I did thought of doing that but that will be a much bigger refactoring, @arun-koshy are you ok with that?

It would actually be much appreciated :)

patrickkuo · 2022-03-15T13:07:11Z

Done refactoring for take and set_server_state, will test for concurrency now.

patrickkuo · 2022-03-15T15:59:23Z

I somehow figured out how to fix take() when I was writing test for the concurrent request, the problem was Mutex, the mutex we use before was sync::Mutex so it doesn't impl Send and that's why it won't compile (so obvious 🙄), things works fine after I change it to future::lock::Mutex.

@666lcz can you check if concurrent request works?

arun-koshy

Thanks so much for doing this Patrick! Looks great

Also incredible find on the real issue that was forcing us to use take()!!!

arun-koshy · 2022-03-15T17:41:56Z

sui/src/rest_server.rs

+    config: NetworkConfig,
+    gateway: GatewayClient,
+    keystore: Arc<RwLock<Box<dyn Keystore>>>,
+    addresses: Vec<SuiAddress>,


nit: Just to be clear it would be good to add a note here that this field is just meant to be used for genesis/start. Maybe renaming it too genesis_addresses is enough? Once the network has been started there should be no expectation of the rest server knowing of any addresses. i.e. no one adding new endpoints should reference this field in server state.

actually every field in ServerState except GatewayClient should be changed or removed when the rest_server -> Gateway transformation is completed

keystore/addresses should be removed as they belongs to wallet/application

working_dir and authority_handles should be removed, genesis and network startup should happen outside of Gateway, i.e. Gateway should not worry about genesis and starting up authorities, it should just connect to it assuming the network exist.

network.conf should become gateway.conf and only contain basic network info for connecting to the authorities, e.g. list of authorities host and port, network timeout, epoch etc...

I have added a TODO to remove these

arun-koshy · 2022-03-15T17:48:26Z

sui/src/rest_server.rs

+    // Taking state object without returning ownership
+    let mut state = server_context.server_state.lock().await;
+    let state = state.as_mut().ok_or_else(|| {
+        custom_http_error(


nit: use server_state_error()

good spot! thanks!

arun-koshy · 2022-03-15T17:56:37Z

sui/src/rest_server.rs

-    for address in addresses.iter() {
-        if let Err(err) = wallet_context.gateway.sync_account_state(*address).await {
-            *server_context.wallet_context.lock().unwrap() = Some(wallet_context);
+    let addresses = state.addresses.clone();


We should aim to remove this as the Rest server should not technically have a store of addresses to return. When /addresses is hit the expectation should be that it hits the GatewayClient every time to get the list of known addresses. Can you switch this to call GatewayClient if it is already available or can you add a TODO for me to do this in a future PR?

hmm get_account is not part of the GatewayAPI, and I think we shouldn't need this endpoint down the line, as the wallet/application should know which address it is managing

arun-koshy · 2022-03-15T18:03:40Z

sui/src/unit_tests/rest_server_tests.rs

+use crate::{create_api, ServerContext};
+
+#[tokio::test]
+async fn test_concurrency() -> Result<(), anyhow::Error> {


Awesome! Thanks for adding a test 😄

666lcz · 2022-03-15T20:44:20Z

I somehow figured out how to fix take() when I was writing test for the concurrent request, the problem was Mutex, the mutex we use before was sync::Mutex so it doesn't impl Send and that's why it won't compile (so obvious 🙄), things works fine after I change it to future::lock::Mutex.

@666lcz can you check if concurrent request works?

So I did

git fetch origin pat/config_refactoring:chris/pat/config
git checkout chris/pat/config
git merge main

to checkout your branch locally, and change the request from

  const objects = [];
        for (const id of objectIds) {
             const info = await this.getObjectInfo(id);
             if (info != null) {
                objects.push(info);
             }
         }

to

const objectsResult = await Promise.all(
            objectIds.map(async (id) => await this.getObjectInfo(id))
 );

The former(sequential) request works but I am seeing the same error for the latter(concurrent) one:

Encounter error for  85BD3DB1803DBE685BDD60AA96E402056DCD1865 ApiError: Failed Dependency
[0]     at /Users/cl/ml/fastnft/nft_mirror/oracle_server/node_modules/openapi-typescript-fetch/src/fetcher.ts:198:15
[0]     at Generator.throw (<anonymous>)
[0]     at rejected (/Users/cl/ml/fastnft/nft_mirror/oracle_server/node_modules/openapi-typescript-fetch/dist/cjs/fetcher.js:6:65)
[0]     at processTicksAndRejections (node:internal/process/task_queues:96:5) {
[0]   headers: Headers {
[0]     [Symbol(map)]: [Object: null prototype] {
[0]       'content-type': [Array],
[0]       'x-request-id': [Array],
[0]       'content-length': [Array],
[0]       date: [Array]
[0]     }
[0]   },
[0]   url: 'http://127.0.0.1:5000/object_info?objectId=85BD3DB1803DBE685BDD60AA96E402056DCD1865',
[0]   status: 424,
[0]   statusText: 'Failed Dependency',
[0]   data: {
[0]     request_id: '1c57a0c9-2e33-4991-b4f2-43c6ec162d65',
[0]     error_code: null,
[0]     message: 'Wallet Context does not exist. Please make a POST request to `sui/genesis/` and `sui/start/` to bootstrap the network.'
[0]   }
[0] }

Feel free to address the concurrent problem as a separate PR.

patrickkuo · 2022-03-16T00:40:43Z

I somehow figured out how to fix take() when I was writing test for the concurrent request, the problem was Mutex, the mutex we use before was sync::Mutex so it doesn't impl Send and that's why it won't compile (so obvious 🙄), things works fine after I change it to future::lock::Mutex.
@666lcz can you check if concurrent request works?

So I did

git fetch origin pat/config_refactoring:chris/pat/config
git checkout chris/pat/config
git merge main

to checkout your branch locally, and change the request from

  const objects = [];
        for (const id of objectIds) {
             const info = await this.getObjectInfo(id);
             if (info != null) {
                objects.push(info);
             }
         }

to

const objectsResult = await Promise.all(
            objectIds.map(async (id) => await this.getObjectInfo(id))
 );

The former(sequential) request works but I am seeing the same error for the latter(concurrent) one:

Encounter error for  85BD3DB1803DBE685BDD60AA96E402056DCD1865 ApiError: Failed Dependency
[0]     at /Users/cl/ml/fastnft/nft_mirror/oracle_server/node_modules/openapi-typescript-fetch/src/fetcher.ts:198:15
[0]     at Generator.throw (<anonymous>)
[0]     at rejected (/Users/cl/ml/fastnft/nft_mirror/oracle_server/node_modules/openapi-typescript-fetch/dist/cjs/fetcher.js:6:65)
[0]     at processTicksAndRejections (node:internal/process/task_queues:96:5) {
[0]   headers: Headers {
[0]     [Symbol(map)]: [Object: null prototype] {
[0]       'content-type': [Array],
[0]       'x-request-id': [Array],
[0]       'content-length': [Array],
[0]       date: [Array]
[0]     }
[0]   },
[0]   url: 'http://127.0.0.1:5000/object_info?objectId=85BD3DB1803DBE685BDD60AA96E402056DCD1865',
[0]   status: 424,
[0]   statusText: 'Failed Dependency',
[0]   data: {
[0]     request_id: '1c57a0c9-2e33-4991-b4f2-43c6ec162d65',
[0]     error_code: null,
[0]     message: 'Wallet Context does not exist. Please make a POST request to `sui/genesis/` and `sui/start/` to bootstrap the network.'
[0]   }
[0] }

Feel free to address the concurrent problem as a separate PR.

Thanks @666lcz !

Are you sure this is running against my branch? The error message Wallet Context does not exist. does not exist in my branch, have you rebuild and restarted the server?

666lcz · 2022-03-16T01:09:54Z

does not exist in my branch, have you rebuild and restarted the server?

Sorry for being an idiot! I was spoiled by the auto restarting feature of the Typescript development lately and forgot to rebuild. Indeed the concurrent requests are working after I rebuilt. Thanks for fixing this!

patrickkuo · 2022-03-16T01:24:13Z

does not exist in my branch, have you rebuild and restarted the server?

Sorry for being an idiot! I was spoiled by the auto restarting feature of the Typescript development lately and forgot to rebuild. Indeed the concurrent requests are working after I rebuilt. Thanks for fixing this!

No problem at all! Glad this fixes the problem!

patrickkuo · 2022-03-16T01:34:55Z

the response of genesis has changed and addresses are returned instead of wallet config, @arun-koshy do we need to notify whoever using the rest api? Will this break anything?

arun-koshy · 2022-03-16T01:38:15Z

the response of genesis has changed and addresses are returned instead of wallet config, @arun-koshy do we need to notify whoever using the rest api? Will this break anything?

Geniteam code is frozen so we should be good to go pushing this to main. I will update our documentation to reflect these changes.

… in rest_server are now transient and won't be persisted to disk, it is already deleting the configs when stop endpoint is call prior the changes.

… take and return of server state object

added concurrent request test for rest server

patrickkuo force-pushed the pat/config_refactoring branch 2 times, most recently from b83c108 to c58a053 Compare March 14, 2022 12:08

patrickkuo requested review from oxade, 666lcz, arun-koshy, lxfind and velvia March 14, 2022 18:12

patrickkuo marked this pull request as ready for review March 14, 2022 18:12

patrickkuo requested a review from huitseeker as a code owner March 14, 2022 18:12

velvia reviewed Mar 14, 2022

View reviewed changes

arun-koshy reviewed Mar 14, 2022

View reviewed changes

arun-koshy mentioned this pull request Mar 14, 2022

[REST Server] support concurrent requests #828

Closed

666lcz reviewed Mar 15, 2022

View reviewed changes

patrickkuo force-pushed the pat/config_refactoring branch from 7081070 to fa34f1d Compare March 15, 2022 16:17

patrickkuo linked an issue Mar 15, 2022 that may be closed by this pull request

[REST Server] support concurrent requests #828

Closed

arun-koshy reviewed Mar 15, 2022

View reviewed changes

arun-koshy approved these changes Mar 16, 2022

View reviewed changes

patrickkuo force-pushed the pat/config_refactoring branch from 354ce9d to c4dc4d3 Compare March 16, 2022 01:33

patrickkuo force-pushed the pat/config_refactoring branch from c4dc4d3 to cf242e6 Compare March 16, 2022 02:11

patrickkuo force-pushed the pat/config_refactoring branch from cf242e6 to 625cbe4 Compare March 16, 2022 13:21

patrickkuo added 9 commits March 16, 2022 15:08

* refactor config for rest-server wallet decoupling

75a290b

refactored rest_server to not use wallet configs and context, configs…

c1f84f0

… in rest_server are now transient and won't be persisted to disk, it is already deleting the configs when stop endpoint is call prior the changes.

encode addresses to hex

1c3566d

refactor config

dc7d065

add comments and revert import reordering

01f2fe6

added with_state and with_state_no_param functions to help manage…

5acc5e6

… take and return of server state object

remove take and remove hacky with_state functions

e028ef0

added concurrent request test for rest server

add license

cb25c08

address PR comments

2c27da9

patrickkuo force-pushed the pat/config_refactoring branch from 625cbe4 to 2c27da9 Compare March 16, 2022 15:09

patrickkuo merged commit 5bc45f9 into main Mar 16, 2022

patrickkuo deleted the pat/config_refactoring branch March 16, 2022 16:04

666lcz mentioned this pull request Mar 16, 2022

Clean up wallet config #811

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wallet + rest server] Remove usage of wallet context in rest server #817

[wallet + rest server] Remove usage of wallet context in rest server #817

patrickkuo commented Mar 13, 2022 •

edited

Loading

patrickkuo commented Mar 14, 2022

velvia left a comment

velvia Mar 14, 2022

patrickkuo commented Mar 14, 2022 •

edited

Loading

arun-koshy Mar 14, 2022

velvia commented Mar 14, 2022

666lcz Mar 15, 2022 •

edited

Loading

666lcz Mar 15, 2022 •

edited

Loading

patrickkuo Mar 15, 2022

666lcz Mar 15, 2022 •

edited

Loading

patrickkuo Mar 15, 2022

arun-koshy Mar 15, 2022

patrickkuo commented Mar 15, 2022

patrickkuo commented Mar 15, 2022

arun-koshy left a comment •

edited

Loading

arun-koshy Mar 15, 2022

patrickkuo Mar 16, 2022

arun-koshy Mar 15, 2022

patrickkuo Mar 16, 2022

arun-koshy Mar 15, 2022

patrickkuo Mar 16, 2022

arun-koshy Mar 15, 2022

666lcz commented Mar 15, 2022

patrickkuo commented Mar 16, 2022

666lcz commented Mar 16, 2022

patrickkuo commented Mar 16, 2022

patrickkuo commented Mar 16, 2022

arun-koshy commented Mar 16, 2022

[wallet + rest server] Remove usage of wallet context in rest server #817

[wallet + rest server] Remove usage of wallet context in rest server #817

Conversation

patrickkuo commented Mar 13, 2022 • edited Loading

patrickkuo commented Mar 14, 2022

velvia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickkuo commented Mar 14, 2022 • edited Loading

Choose a reason for hiding this comment

velvia commented Mar 14, 2022

666lcz Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

666lcz Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

666lcz Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickkuo commented Mar 15, 2022

patrickkuo commented Mar 15, 2022

arun-koshy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

666lcz commented Mar 15, 2022

patrickkuo commented Mar 16, 2022

666lcz commented Mar 16, 2022

patrickkuo commented Mar 16, 2022

patrickkuo commented Mar 16, 2022

arun-koshy commented Mar 16, 2022

patrickkuo commented Mar 13, 2022 •

edited

Loading

patrickkuo commented Mar 14, 2022 •

edited

Loading

666lcz Mar 15, 2022 •

edited

Loading

666lcz Mar 15, 2022 •

edited

Loading

666lcz Mar 15, 2022 •

edited

Loading

arun-koshy left a comment •

edited

Loading