[ECO-486][ECO-538] Add balance update support #9

alnoki · 2023-10-02T09:41:25Z

Delete deprecated Rust README
Add processor for MarketAccounts table handles
Add missing postgres views to old migrations
Add processor for MarketAccount table entries
Add view function to join on table handle for balance updates

To verify

health_check_port: 8085
server_config:
  processor_config:
    type: econia_transaction_processor
    econia_address: 0xc0de11113b427d35ece1d8991865a941c0578b0f349acabbe9753863c24109ff
  postgres_connection_string: postgres://econia:econia@postgres:5432/econia
  indexer_grpc_data_service_address: https://grpc.testnet.aptoslabs.com:443
  indexer_grpc_http2_ping_interval_in_secs: 60
  indexer_grpc_http2_ping_timeout_in_secs: 10
  auth_token: <my_auth_token>
  starting_version: 683453241

Run Docker compose instructions per the docs site then see http://0.0.0.0:3001/balance_updates

elliottdehn · 2023-10-02T15:39:28Z

rust/processor/src/processors/econia_processor.rs

+                                .expect("Failed to parse MarketAccounts");
+                            market_account_handles.push(MarketAccountHandle {
+                                user: resource.address.clone(),
+                                handle: data["map"]["handle"].as_str().unwrap().to_string(),


See comment on econia-labs/econia#511, I think we should include a time field here so that we can know what time a user's accounts were first active.

Included as of 306a3cb

CRBl69 · 2023-10-02T15:18:51Z

rust/processor/src/processors/econia_processor.rs

+                            base_total: u64::from_str(data["base_total"].as_str().unwrap())
+                                .unwrap()
+                                .into(),
+                            base_available: u64::from_str(data["base_available"].as_str().unwrap())
+                                .unwrap()
+                                .into(),
+                            base_ceiling: u64::from_str(data["base_ceiling"].as_str().unwrap())
+                                .unwrap()
+                                .into(),
+                            quote_total: u64::from_str(data["quote_total"].as_str().unwrap())
+                                .unwrap()
+                                .into(),
+                            quote_available: u64::from_str(
+                                data["quote_available"].as_str().unwrap(),
+                            )


I'd like to avoid the use of unwrap just like above in the code. I'd add a function just like opt_value_to_{string,bool,big_decimal} but for a u64 from str just for this (opt_value_str_to_u64) that just returns a result.

Unwraps removed per 306a3cb

CRBl69 · 2023-10-02T15:58:05Z

rust/processor/src/processors/econia_processor.rs

+                        let table_data = write.data.as_ref().expect("No WriteTableItem data");
+                        if table_data.value_type
+                            != format!("{}::user::MarketAccount", econia_address)
+                        {
+                            continue;
+                        }
+                        let table_key: serde_json::Value = serde_json::from_str(&table_data.key)
+                            .expect("Failed to parse market account ID");
+                        let market_account_id =
+                            u128::from_str(table_key.as_str().unwrap()).unwrap();
+                        let data: serde_json::Value = serde_json::from_str(&table_data.value)
+                            .expect("Failed to parse MarketAccount");


Let's avoid unwrap here as well. See opt_value_to_* for examples on how to do that.

Unwraps removed per 306a3cb

elliottdehn · 2023-10-02T22:17:21Z

rust/processor/src/processors/econia_processor.rs

+                            .expect("Failed to parse MarketAccount");
+                        balance_updates.push(BalanceUpdate {
+                            txn_version: txn_version.into(),
+                            handle: write.handle.to_string(),


We should include the user id associated with the handle in this table, so that events notifying balance updates contain it--the handle is kind of useless without referring to the other table. Since the ID associated with the handle doesn't change, we can save the look-up and make things easier for people by including it in this table too.

You might need to pre-process all of the WriteResource events.

Suggesting this as it feeds into/affects ECO-578

This is accounted for via the balance_updates natural join view (see the associated up.sql migration).

market_account_handles:

balance_updates_by_handle:

balance_updates (the natural join view function):

We can't attach notifications to that view though (at least I don't think so) and cross-referencing every event to that view in order to get the user address is both expensive (it's a full table join) and unnecessary (the data can be included).

Are you expecting that people will perform a full table join every time they get a balance update event in order to get the user's address?

There might be tens of thousands of balance updates in that table, and remember people will be running this is GCP or whatever provider so they'll be getting billed for that work every trade.

There are multiple possible solutions here, none of which require that the processor cache extra data or read from the database:

Create a trigger that simply sends out balance_update_by_handle entries verbatim. In this case, the listener simply has to look up their handle in market_account_handles one time and then knows which events to retain in the future

Create a trigger that sends out the equivalent of an entry in balance_updates, but without doing a join. The function needs to take a balance_updates_by_handle entry and lookup the handle in market_account_handles, then substitute in the corresponding user field. This can be done purely in migrations, though it will require that the db insertion does market account handle insertions before balance updates (currently they happen after per the build_transaction().

I lean toward (1) as it eliminates internal dependencies and is a simple trigger

If we go with option 1 then I think we should just get rid of the natural join view. I don't think it has such value or utility that it should be part of the API surface area.

alnoki added 8 commits October 1, 2023 20:24

Add market account handles processor

fc4719c

Fix deserialization issues

40fd005

Revert to pure string type

e916098

Change JSON access method for table handle

c25d34f

Use direct string type deserializer

07484b0

Add balance updates processor

d141053

Change u128 deserialization method

e046d97

Change u64 conversion style

a5f8d3a

alnoki changed the title ~~[ECO-486][ECO-535] Add balance update support~~ [ECO-486][ECO-538] Add balance update support Oct 2, 2023

elliottdehn reviewed Oct 2, 2023

View reviewed changes

CRBl69 requested changes Oct 2, 2023

View reviewed changes

alnoki added 4 commits October 2, 2023 14:06

Merge branch 'econia' into ECO-486

0803dde

Remove unwraps

f8dd5b3

Fix compilation errors

306a3cb

Change table key parse method

4bb7c5a

elliottdehn suggested changes Oct 2, 2023

View reviewed changes

alnoki marked this pull request as draft October 2, 2023 22:20

alnoki marked this pull request as ready for review October 2, 2023 22:42

alnoki marked this pull request as draft October 2, 2023 22:50

alnoki marked this pull request as ready for review October 2, 2023 22:54

elliottdehn approved these changes Oct 2, 2023

View reviewed changes

CRBl69 approved these changes Oct 3, 2023

View reviewed changes

alnoki merged commit 1a68026 into econia Oct 3, 2023
1 of 5 checks passed

alnoki deleted the ECO-486 branch October 3, 2023 19:07

alnoki mentioned this pull request Oct 4, 2023

[ECO-484] Add order book state econia-labs/econia#518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ECO-486][ECO-538] Add balance update support #9

[ECO-486][ECO-538] Add balance update support #9

alnoki commented Oct 2, 2023 •

edited

Loading

elliottdehn Oct 2, 2023 •

edited

Loading

alnoki Oct 2, 2023

CRBl69 Oct 2, 2023

alnoki Oct 2, 2023

CRBl69 Oct 2, 2023

alnoki Oct 2, 2023

elliottdehn Oct 2, 2023 •

edited

Loading

alnoki Oct 2, 2023 •

edited

Loading

elliottdehn Oct 2, 2023 •

edited

Loading

alnoki Oct 2, 2023

elliottdehn Oct 2, 2023

[ECO-486][ECO-538] Add balance update support #9

[ECO-486][ECO-538] Add balance update support #9

Conversation

alnoki commented Oct 2, 2023 • edited Loading

To verify

elliottdehn Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

alnoki Oct 2, 2023

Choose a reason for hiding this comment

CRBl69 Oct 2, 2023

Choose a reason for hiding this comment

alnoki Oct 2, 2023

Choose a reason for hiding this comment

CRBl69 Oct 2, 2023

Choose a reason for hiding this comment

alnoki Oct 2, 2023

Choose a reason for hiding this comment

elliottdehn Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

alnoki Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

elliottdehn Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

alnoki Oct 2, 2023

Choose a reason for hiding this comment

elliottdehn Oct 2, 2023

Choose a reason for hiding this comment

alnoki commented Oct 2, 2023 •

edited

Loading

elliottdehn Oct 2, 2023 •

edited

Loading

elliottdehn Oct 2, 2023 •

edited

Loading

alnoki Oct 2, 2023 •

edited

Loading

elliottdehn Oct 2, 2023 •

edited

Loading