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: added block handler to handle indexing of reserve balance #4

Closed
wants to merge 2 commits into from

Conversation

frazarshad
Copy link
Collaborator

@frazarshad frazarshad commented Apr 25, 2024

This PR adds a block handler to the subql node.
This block handler is responsible for querying the total ist balance in reserve and indexing that value

@frazarshad frazarshad requested a review from toliaqat April 25, 2024 17:02
const response = await crossFetch(url);

if (response.status >= 400) {
const errorText = await response.text();
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of checking the response status? What if it's in the 300 range?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use try/catch? That way you don't need to check for response status on line 104? If there is an error, the catch block can throw an error accordingly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

crossFetch wont automatically throw an error in cases where it receives a proper response. such as when we hit this URL https://main-a.api.agoric.net/cosssmos/auth/v1beta1/module_accountss
We receive a 501 but it doesnt throw an error. in those cases we manually check if we have an error code and throw an error ourselves

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What if it's in the 300 range?

3XX are for redirect codes which are not errrors

Copy link
Collaborator

Choose a reason for hiding this comment

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

crossFetch wont automatically throw an error in cases where it receives a proper response. such as when we hit this URL https://main-a.api.agoric.net/cosssmos/auth/v1beta1/module_accountss

Interesting. I reckon this is something wrong with the endpoint we're hitting, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

3XX are for redirect codes which are not errrors

Yeah. However, it does not indicate a successful response. Perhaps we should handle it too.

Copy link
Collaborator Author

@frazarshad frazarshad Apr 26, 2024

Choose a reason for hiding this comment

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

Interesting. I reckon this is something wrong with the endpoint we're hitting, right?

This is the case with most API responses actually. a lot of libraries dont throw an error implicitly and expect you to do it yourself

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. However, it does not indicate a successful response. Perhaps we should handle it too.

300 are successful responses in the sense that we can actually obtain useful responses from them. compared to 400s and 500s which just represent a failure

Copy link
Collaborator

Choose a reason for hiding this comment

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

300 are successful responses in the sense that we can actually obtain useful responses from them. compared to 400s and 500s which just represent a failure

In that regard, 400 and 500 are also useful responses converting us what's wrong :D

Copy link
Contributor

Choose a reason for hiding this comment

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

@rabi-siddique we look for errors in 400+ range. Here we are just checking for errors in response.

@@ -37,6 +37,7 @@ services:
- --db-schema=app
- --workers=4
- --batch-size=30
- --unsafe
# - --log-level=debug
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why unsafe? Isn't it unsafe? :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

subql uses a restricted version of node to create a controlled env for running the code. sort of like endojs for hardened js

Therefore a lot of functionality (such as making api calls) is restricted. to enable it you need to run it in unsafe mode. you can read more about it here

@@ -46,6 +46,7 @@
"dependencies": {
"@subql/types-cosmos": "^3.2.3",
"@types/node": "^17.0.21",
"cross-fetch": "^4.0.0",
"pino": "^7.8.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use axios?

After comparing axios and cross-fetch, I think axios has more features compared to cross-fetch. It's easier to use, has automatic JSON parsing, and more popular :D

cross-fetch does have the advantage of being lightweight at 9.8kB minified, compared to axios's 30.5kB.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

axios unfortunately does not work in the restricted environment properly. i've tried a node-fetch as well which did not work

only cross-fetch seems to work properly

type ReserveBalance @entity {
id: ID!
blockHeight: BigInt!
address: String!
Copy link
Collaborator

Choose a reason for hiding this comment

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

@toliaqat I am not sure about it. But what if the blockHeight is much larger than what a BigInt type can handle? Should we also use a String type for blockHeight?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BigInt can store super long numbers such as: 10 ^(10^8) which i dont think we can physically reach

Copy link
Contributor

Choose a reason for hiding this comment

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

BigInt is good enough.

@@ -40,6 +42,22 @@ BigInt.prototype.toJSON = function () {
return this.toString();
};

const API_ENDPOINT = 'https://main-a.api.agoric.net:443'
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a parameter in project.js called network which has an endpoint URL list. Project.js is complied into project.yaml. Is there a was way to read network.endpoint list instead of creating another variable here?

const response = await crossFetch(url);

if (response.status >= 400) {
const errorText = await response.text();
Copy link
Contributor

Choose a reason for hiding this comment

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

@rabi-siddique we look for errors in 400+ range. Here we are just checking for errors in response.

Copy link
Contributor

@toliaqat toliaqat left a comment

Choose a reason for hiding this comment

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

The more I think, i feel the API call for each block will slow down our indexing. I think instead of indexing this reserve balance, let's call the API in the inter-dashboard frontend when rendering the Reserve page. What do you think?

@frazarshad
Copy link
Collaborator Author

The more I think, i feel the API call for each block will slow down our indexing. I think instead of indexing this reserve balance, let's call the API in the inter-dashboard frontend when rendering the Reserve page. What do you think?

@toliaqat while i think thats a fair point, do you think it would be inconsistent that some of our api calls are made to the indexer while some are made to the chain

@toliaqat
Copy link
Contributor

@toliaqat while i think thats a fair point, do you think it would be inconsistent that some of our api calls are made to the indexer while some are made to the chain

True, and this can be resolved when we have closed this issue #7
For now, I believe, we cannot afford to make our indexing slow down.
All blocks should all the data that we can index if done properly without calling the chain's RPC directly from this code base.

@toliaqat toliaqat closed this Apr 30, 2024
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