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.

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

Improve query balance #2886

Closed
wants to merge 2 commits into from
Closed

Improve query balance #2886

wants to merge 2 commits into from

Conversation

duyhla
Copy link

@duyhla duyhla commented Mar 12, 2024

Describe your changes

Upon reviewing our codebase, I've noticed that querying transparent balances might not always be necessary, and it could significantly slow down the overall process due to the large amount of data involved. Therefore, I suggest removing the section of code responsible for querying transparent balances entirely.

By removing the call to query_transparent_balance and query_shielded_balance, we can potentially improve the overall performance of our querying process, especially when dealing with large datasets. This optimization ensures that we only query the balances that are essential for our operations, thus reducing unnecessary processing time and resource consumption.

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.88%. Comparing base (cc3edde) to head (a9a2871).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2886   +/-   ##
=======================================
  Coverage   53.88%   53.88%           
=======================================
  Files         308      308           
  Lines      100154   100152    -2     
=======================================
- Hits        53967    53966    -1     
+ Misses      46187    46186    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@quangtuyen88
Copy link

I think the logic implemented now is correct.

The function uses a match statement to determine the type of the owner and calls the appropriate function to query the balance.
If the owner is a full viewing key, the function query_shielded_balance is called to query the balance of a shielded account.
If the owner is an address, the function query_transparent_balance is called to query the balance of a transparent account.
If the owner is a payment address, the function query_pinned_balance is called to query the balance of a pinned account.

If the owner is not provided (None), the function query_pinned_balance is called first to query the balance of the pinned account for the token symbol provided in args. Then, query_shielded_balance is called to query the balance of a shielded account. Finally, query_transparent_balance is called to query the balance of a transparent account. All these balances are queried for the token symbol provided in args.

Each of these functions (query_shielded_balance, query_transparent_balance, query_pinned_balance) is an asynchronous function that queries the balance from the Namada blockchain and handles any potential errors.

You can't just remove query query_shielded_balance + query_transparent_balance if you don't understand logic.

@duyhla
Copy link
Author

duyhla commented Mar 12, 2024

I understand the importance of preserving the logic and functionality of the code. However, my concern primarily lies in the efficiency of querying balances, especially when dealing with potentially large datasets. While I appreciate the need to query all wallet balances, I believe it's essential to optimize this process to minimize unnecessary resource consumption and potential performance issues.

By optimizing the querying process, we can ensure that we retrieve only the necessary data, leading to improved performance and a better user experience. I hope this clarifies my perspective, and I'm open to further discussion on how we can balance efficiency with functionality in our codebase.

@duyhla
Copy link
Author

duyhla commented Mar 12, 2024

With the current test scenario already exhibiting performance challenges, scaling up to a million accounts on the mainnet would likely exacerbate these issues significantly.

Drawing a comparison to the existing shield-sync Namada-extension further emphasizes the potential pitfalls of such a scenario. We've observed how the shield-sync extension can face difficulties and delays in synchronizing large amounts of data, and we must learn from these experiences to design more efficient and scalable solutions for querying balances.

My expertise in DevOps and cloud infrastructure cost analysis adds another critical dimension to this discussion. As you rightly pointed out, in addition to the time and resource consumption, querying large amounts of data also incurs substantial network costs, especially when utilizing cloud services like AWS.

With AWS pricing for data processing at $0.045 per gigabyte, the expense of querying large datasets can quickly add up. Given the potential scale of our application, querying balances for millions of accounts could result in significant network costs, further underscoring the importance of optimizing our querying mechanisms to minimize unnecessary data transfer.

@quangtuyen88
Copy link

quangtuyen88 commented Mar 12, 2024

I can understand what you mean about transfer cost or resources will be used.. But there is many way can reduce transfer cost for that ( like cache req use CDN) .. .The fact is if you don't pass --owner your_wallet_address/alias it should query all balances accounts. The logic is if it's a bug you can suggest team force the client to add --owner to query the balance instead of scanning all the balance ( I'm not sure if only balance exists on your local server or not).

@brentstone
Copy link
Collaborator

Appreciate the discussion here, but I'll be closing this PR since the very few code changes in here don't do anything. Fwiw, we plan to improve the balance querying anyway before mainnet, see #3014.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants