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

Feat: saving last block query for next fetching #716

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

huvny-de
Copy link

@huvny-de huvny-de commented Apr 2, 2024

Issue

#662

Solution

Depend on new version of SDK is Namada 0.32.0, to minimize block fetching during shielded syncing, the approach involves passing both the start_query_height and last_query_height parameters fetch function, thus restricting fetching to a specified range. To ensure the correct values for these parameters, the following solution can be implemented:

  • Retrieve the start_query_height from local storage.
  • Initiate a new function to obtain the latest block, which will determine the last_query_height as the latest block on the network.
  • Sync only within the range defined by these two values.
  • Upon completion of the sync process, store the value of the latest block in local storage to facilitate subsequent shielded syncing.

@emccorson emccorson added the needs discussion in issue Issue needs to be discussed before PR can be reviewed label Apr 3, 2024
@Wiemsdorfba
Copy link

Tks @huvny-de for providing this solution. It is indeed necessary to optimize the synchronization process, and this solution effectively addresses the issue by minimizing unnecessary data fetching and storing essential parameters for future synchronization

This solution optimizes shielded syncing, minimizing resource usage while ensuring accurate and efficient data retrieval from the Namada network

@mateuszjasiuk mateuszjasiuk linked an issue Apr 4, 2024 that may be closed by this pull request
@mateuszjasiuk mateuszjasiuk self-requested a review April 4, 2024 08:49
Copy link
Collaborator

@mateuszjasiuk mateuszjasiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think this is a good start for shielded sync improvements. Left few comments :)

@@ -185,6 +185,8 @@ const accountsAtom = (() => {
);
})();

const LATEST_SYNCED_HEIGHT_STORAGE = "latest-synced-height"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think last block info should be stored in extension local storage.

export class LocalStorage extends ExtStorage {

await this._namada?.shieldedSync({ startHeight, lastHeight });
}

public async queryLastBlock(): Promise<number | undefined> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once you use extension local storage this function is not needed anymore :)

None,
None,
start_query_height,
last_query_height,
Copy link
Collaborator

@mateuszjasiuk mateuszjasiuk Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure Namada will fetch up to last block if you pass None.

const startHeightStorage = Number(localStorage.getItem(LATEST_SYNCED_HEIGHT_STORAGE));
const startHeight = startHeightStorage ? startHeightStorage : undefined;
await namada.sync(startHeight,lastHeight);
if(lastHeight){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like formatting is a bit off. Please run yarn prepare in the root of monorepo to setup husky.

@@ -278,12 +291,22 @@ impl Query {

let _ = shielded.save().await?;

let start_query_height = match start_height {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also just start_height.map(BlockHeight) - should work :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha. Thanks for your suggestion. I'm a newbie on Rust

@mateuszjasiuk mateuszjasiuk added improvement App: Extension Lib: SDK and removed needs discussion in issue Issue needs to be discussed before PR can be reviewed labels Apr 4, 2024
@huvny-de
Copy link
Author

huvny-de commented Apr 8, 2024

Hi @mateuszjasiuk , Thanks for your comment. I fixed all of it. Please take a look on that.

Copy link
Collaborator

@mateuszjasiuk mateuszjasiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general it looks good. I've tested it locally and it seems to be working nicely :)
There are few more things to change but they should be easy.
First, linting errors (you can run yarn lint in the root of the monorepo to see them):

@namada/extension
 | 
 | /home/mj/Projects/heliax/namada-interface/apps/extension/src/background/keyring/keyring.ts
 |   819:3  error  Missing return type on function  @typescript-eslint/explicit-function-return-type
 | 
 | ✖ 1 problem (1 error, 0 warnings)
 | 
 | `yarn lint:ci` failed with exit code 1
@namada/sdk
 | 
 | /home/mj/Projects/heliax/namada-interface/packages/sdk/src/rpc/rpc.ts
 |   215:3  warning  Missing JSDoc @param "start_height" declaration  jsdoc/require-param
 | 
 | ✖ 1 problem (0 errors, 1 warning)
 |   0 errors and 1 warning potentially fixable with the `--fix` option.

Also, we need to reset lastestSyncBlock every time we import new account. The reason is we might import an account that was used for shielding in block 10, but latestSyncBlock is 20 or whatever. So to get accurate context we need to sync from the beginning unfortunately :(

?.lastestSyncBlock;
const lastHeight = await this.queryLastBlock();

if (startHeight !== undefined && lastHeight) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this "if" after the shieldedSync call. This way you can be sure that we completed the sync before saving lastBlock info

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

Successfully merging this pull request may close these issues.

Fetching shielded balance via SDK nor extension working
4 participants