[enhancement] store balances at block heights - implementation #81

joel-u410 · 2024-07-19T20:47:49Z

Pushing this as draft, in order to show my proposed changes here. This cannot be merged until the corresponding feature is merged in the SDK: anoma/namada#3530. After that's done, I'll come back here and update this to use the upstream SDK and not my fork.

This implements #77.

Example of the balance_changes table:

And corresponding balances view (which maintains the previous behavior of the balances table):

After a transaction occurs which updates the balance (note the id changes in the returned row in the balances view to reflect which balance is now the latest one):

joel-u410 · 2024-09-09T16:56:50Z

Good news -- anoma/namada#3530 was merged, so I suppose after the next namada SDK release, this can be updated / rebased and will no longer require a forked version.

mateuszjasiuk · 2024-10-31T13:20:18Z

@joel-u410 👋 Do you still need this? I think your PR to namada was merged some time ago

joel-u410 · 2024-10-31T17:05:16Z

@joel-u410 👋 Do you still need this? I think your PR to namada was merged some time ago

Hi @mateuszjasiuk yes I'd still like this change to get merged (though it is out of date and needs some work now!)

I just haven't had the chance to come back to this project yet (have been waiting until closer to mainnet launch to refocus here). Please keep it open, if it's not too much trouble, and I will update/rebase this soon (probably next week) based on the current namada SDK which does have my PR merged now. Thanks!

mateuszjasiuk · 2024-10-31T19:40:33Z

@joel-u410 👋 Do you still need this? I think your PR to namada was merged some time ago

Hi @mateuszjasiuk yes I'd still like this change to get merged (though it is out of date and needs some work now!)

I just haven't had the chance to come back to this project yet (have been waiting until closer to mainnet launch to refocus here). Please keep it open, if it's not too much trouble, and I will update/rebase this soon (probably next week) based on the current namada SDK which does have my PR merged now. Thanks!

Sure, no problem!

… wrapper_transactions

This reverts commit b6aa705.

…height

…ry limits

joel-u410 · 2024-11-21T18:27:42Z

@mateuszjasiuk I've rebased this now + resolved conflicts on latest main -- should be ready to go! 🙏

I was not able to run cargo test locally (it gave errors about the test database not existing) -- is there something I'm missing there? (I used docker-compose-db.yml to run a local database and set DATABASE_URL_TEST to point to it).

Would like to make sure all tests are passing before this is merged.

joel-u410 · 2024-11-21T18:29:24Z

re: running cargo test -- I created a database namada-indexer-test and my DATABASE_URL_TEST points to it. I get errors like this:

    0: Failed to establish connection: connection to server at "0.0.0.0", port 5433 failed: FATAL:  database "namada-indexer-test/test_db_1535_21" does not exist
    1: connection to server at "0.0.0.0", port 5433 failed: FATAL:  database "namada-indexer-test/test_db_1535_21" does not exist

It seems like I'm missing something here where the tests are supposed to have a new database for each test run?

mateuszjasiuk · 2024-11-21T18:52:19Z

You can try this: DATABASE_URL_TEST=postgres://postgres:[email protected]:5433 cargo test I'm not sure why cargo test is not picking up env. I did not have time to check it out 😅

joel-u410 · 2024-11-22T17:09:46Z

ah, I see ... DATABASE_URL_TEST must not specify a database name. Thanks, that worked.

joel-u410 · 2024-11-22T17:10:44Z

ok, so there is one failing test on this branch. I'll update & make sure that's passing before marking this as ready.

…at the same height

joel-u410 · 2024-11-25T20:46:03Z

@mateuszjasiuk this is ready for review now. I updated the test test_insert_balance_with_conflicting_owners to be more correct, I think, based on making it more like test_insert_balance_with_conflicting_tokens -- actually inserting a new token. As it was written, it was actually just a duplicate of test_insert_balance_with_existing_balances_update.

I also added another test case to test the behavior when a different balance amount is inserted with the same owner, token, and height (the new balance in that case is ignored).

mateuszjasiuk

Tested and I think this looks great. Left couple of comments for discussion, but in general awesome work!

EDIT: also my small concern is that up until this point we would just store current state, so if someone starts indexing late then the balance changes will start at block x not first one. If we provide new endpoint to get balance at height we probably need to mention this in the description

mateuszjasiuk · 2024-11-26T09:31:41Z

chain/src/repository/balance.rs

+    tracing::info!("Inserting {} balances into db", balances.len());
+
+    // Group balances into chunks to avoid hitting the limit of the number of bind parameters in one query.
+    balances.chunks(10000).try_for_each(|balances_chunk| {


I'm not sure if it fits your usecase but we also have insert_balance_in_chunks right below this function

mateuszjasiuk · 2024-11-26T09:32:44Z

chain/src/repository/balance.rs

        };

        db.run_test(move |conn| {
            seed_balance(conn, vec![balance.clone()])?;

+            // TODO: this is probably not a valid way to construct an IbcToken


I think it's fine to remove TODO but leave comment

mateuszjasiuk · 2024-11-26T09:35:03Z

orm/migrations/2024-11-25-203433_add_height_to_balances/up.sql

+            OWNER,
+            token) max_heights ON bc.owner = max_heights.owner
+    AND bc.token = max_heights.token
+    AND bc.height = max_heights.max_height;


We do breaking changes all the time before mainnet launch, so you can also just change init_balances migration if you like to simplify the PR :)

agree, if you could avoid having the migration and modify directly the balance table would be great! can keep the view, thanks!

mateuszjasiuk · 2024-11-26T09:35:52Z

transactions/src/repository/transactions.rs

@@ -18,6 +18,7 @@ pub fn insert_inner_transactions(
                .map(InnerTransactionInsertDb::from)
                .collect::<Vec<_>>(),
        )
+        .on_conflict_do_nothing()


Can you give some context why this is needed, I think conflict should never happen 🤔

joel-u410 force-pushed the joel/balances-at-height branch from 0bb5d21 to e60b7ba Compare July 19, 2024 21:53

joel-u410 force-pushed the joel/balances-at-height branch from e60b7ba to fc01750 Compare August 22, 2024 19:29

joel-u410 added 6 commits November 21, 2024 11:24

[transactions] ignore conflicts when inserting inner_transactions and…

70ba863

… wrapper_transactions

[Cargo.toml] patch namada core to use u410 fork

b6aa705

Revert "[Cargo.toml] patch namada core to use u410 fork"

1b054c5

This reverts commit b6aa705.

[chain] Query balances at block_height

eb7df3a

[enhancement] balances -> balance_changes, record balance history by …

a0f4cd4

…height

[chain] When inserting balances, do it in chunks to avoid hitting que…

15dd4fa

…ry limits

joel-u410 force-pushed the joel/balances-at-height branch from 2b76c49 to 15dd4fa Compare November 21, 2024 18:15

joel-u410 added 2 commits November 25, 2024 13:42

[migrations] Rename to todays date

2452808

[chain/test] Update balance conflict tests to be more valid

19281f8

joel-u410 marked this pull request as ready for review November 25, 2024 20:31

[chain/test] Add test for behavior when inserting new balance amount …

e1d22c4

…at the same height

mateuszjasiuk reviewed Nov 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[enhancement] store balances at block heights - implementation #81

[enhancement] store balances at block heights - implementation #81

joel-u410 commented Jul 19, 2024 •

edited

Loading

joel-u410 commented Sep 9, 2024

mateuszjasiuk commented Oct 31, 2024

joel-u410 commented Oct 31, 2024

mateuszjasiuk commented Oct 31, 2024

joel-u410 commented Nov 21, 2024

joel-u410 commented Nov 21, 2024

mateuszjasiuk commented Nov 21, 2024

joel-u410 commented Nov 22, 2024 •

edited

Loading

joel-u410 commented Nov 22, 2024

joel-u410 commented Nov 25, 2024 •

edited

Loading

mateuszjasiuk left a comment •

edited

Loading

mateuszjasiuk Nov 26, 2024

mateuszjasiuk Nov 26, 2024

mateuszjasiuk Nov 26, 2024

Fraccaman Nov 26, 2024

mateuszjasiuk Nov 26, 2024

[enhancement] store balances at block heights - implementation #81

Are you sure you want to change the base?

[enhancement] store balances at block heights - implementation #81

Conversation

joel-u410 commented Jul 19, 2024 • edited Loading

joel-u410 commented Sep 9, 2024

mateuszjasiuk commented Oct 31, 2024

joel-u410 commented Oct 31, 2024

mateuszjasiuk commented Oct 31, 2024

joel-u410 commented Nov 21, 2024

joel-u410 commented Nov 21, 2024

mateuszjasiuk commented Nov 21, 2024

joel-u410 commented Nov 22, 2024 • edited Loading

joel-u410 commented Nov 22, 2024

joel-u410 commented Nov 25, 2024 • edited Loading

mateuszjasiuk left a comment • edited Loading

Choose a reason for hiding this comment

mateuszjasiuk Nov 26, 2024

Choose a reason for hiding this comment

mateuszjasiuk Nov 26, 2024

Choose a reason for hiding this comment

mateuszjasiuk Nov 26, 2024

Choose a reason for hiding this comment

Fraccaman Nov 26, 2024

Choose a reason for hiding this comment

mateuszjasiuk Nov 26, 2024

Choose a reason for hiding this comment

joel-u410 commented Jul 19, 2024 •

edited

Loading

joel-u410 commented Nov 22, 2024 •

edited

Loading

joel-u410 commented Nov 25, 2024 •

edited

Loading

mateuszjasiuk left a comment •

edited

Loading