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

Helper functions to simplify writing social indexers. #381

Closed

Conversation

gabehamilton
Copy link
Collaborator

@gabehamilton gabehamilton commented Nov 9, 2023

@gabehamilton gabehamilton requested a review from a team as a code owner November 9, 2023 23:21
@gabehamilton gabehamilton linked an issue Nov 9, 2023 that may be closed by this pull request
@gabehamilton
Copy link
Collaborator Author

Please weigh in on naming.
Currently the helper functions are named:

base64Decode
getFunctionCalls
getSocialOperations

One alternative would be to add the extraction methods to the block instead of the context and call them

functionCalls
socialOperations

Copy link
Collaborator

@darunrs darunrs left a comment

Choose a reason for hiding this comment

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

The functions look very helpful! I'd love to rewrite social feed with them, to get that indexer even shorter. Left some comments around error handling. I'd love to see some unit tests too, to ensure the new functions are error tolerant.

.filter((operation) => operation?.methodName === method)
.map((functionCallOperation) => ({
...functionCallOperation,
args: base64DecodeFunction(functionCallOperation.args),
Copy link
Collaborator

@darunrs darunrs Nov 10, 2023

Choose a reason for hiding this comment

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

It might be possible for the decode to fail depending on the structure of args. I vaguely recall a block which failed there. I would wrap this in a try catch and return an empty result. The user is not able to control the contents of a block or this function code. So, if it throws errors, they need to be aware that some blocks will crash their code and stop their indexer if they don't handle the error. Or we can just handle it, which I feel is the better option.

db: this.buildDatabaseContext(account, schemaName, schema, blockHeight),
base64Decode: base64DecodeFunction,
getFunctionCalls: getFunctionCallsFunction,
getSocialOperations: (block, operation) => {
Copy link
Collaborator

@darunrs darunrs Nov 10, 2023

Choose a reason for hiding this comment

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

Fortunately, I do actually have a test block for this piece of code. The block 100505912 has social posts with malformed data. As a result, the below filter and map functions will actually fail, throwing an uncaught error that will stop your indexer. You can run that block against the indexer you made and see that it does in fact fail. The fix I did was to simply wrap the calls in a try catch. You can see the fix here: https://github.com/near/near-discovery-components/blob/develop/indexers/social_feed/social_feed.js#L271-L283

Comment on lines +207 to +209
const contract = 'social.near';
const method = 'set';
return getFunctionCallsFunction(block, contract, method)
Copy link
Collaborator

@darunrs darunrs Nov 10, 2023

Choose a reason for hiding this comment

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

nit: Can't we simply put the contract and method in the function call, since they're constants anyway? Otherwise, if we want to keep the variables to declare the constants, we should place them as a class level constant with the names SOCIAL_CONTRACT and SET_METHOD. Or at least rename the two you have here to that.

@darunrs
Copy link
Collaborator

darunrs commented Nov 10, 2023

Regarding the naming, they look fine to me. For the socialOperations one, I have a question. The name implies there are different operations you can execute on the social.near contract. I have only seen the set method. What other methods are used commonly?

I think I would like to keep socialOperations in this. But decode and get functions could possibly go in Block. I'd say we can keep the PR as is for now and migrate if we want to, when we look into supporting things like other Actions.

@gabehamilton
Copy link
Collaborator Author

Regarding the naming, they look fine to me. For the socialOperations one, I have a question. The name implies there are different operations you can execute on the social.near contract. I have only seen the set method. What other methods are used commonly?

Maybe this needs a naming discussion as well. It's not the method it's the top-level data element being stored. post, graph, settings, widget, profile, anythings else anyone decides to store. index is a special one. The next level below it are like, notify, star, anything someone decides to store. Those get indexed by api-server-js.

Maybe this should be called path and arguments like /index/like would match operations where index: { like: data is being set?

@darunrs
Copy link
Collaborator

darunrs commented Nov 10, 2023

Gotcha. My comment was mainly around if it should be socialOperations or socialPosts (as social post is what its called in the social feed indexer). But, from what you just explained, socialOperations is probably better. I think path would make it seem more like an API. I think going as far as paths is reaching a bit too far. We would need to keep that up to date. I would say its better to let the user handle parsing the data (such as seeing if a post contains a like, or a post) themselves. We just strip out parts of the block that, if you call the function, we know you don't want/need.

@pkudinov
Copy link
Collaborator

Adding these methods to context means we have to support them perpetually until the last indexer upgrades to another solution. They all (besides the social methods) belong to near primitives, so we should prioritize adding them there instead.

For socialOperations, I think this deserves to be a separate library altogether, because this is not generic enough and does not adhere to any NEP standard, and therefore subject to change. So maintaining it inside near-primitives will involve version management, migrations, etc.

Perhaps for now you can keep them as tidy snippet of code you keep in gist and docs, so that users can copy-paste it into their indexers?

@pkudinov pkudinov self-requested a review November 21, 2023 23:00
@morgsmccauley
Copy link
Collaborator

To be moved to near-lake-primitives

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.

Social operations context helper functions
4 participants