refactor: suggested changes for optimization and code organisation #133
Conversation
Other than one time - non avoidable RPC calls to create |
| getOrCreateAccountVTokenTransaction, | ||
| getOrCreateComptroller, | ||
| getOrCreateMarket, | ||
| } from '../operations/getOrCreate'; |
There was a problem hiding this comment.
It looks like we lost some things here like update and create functions.
The update helpers ideally wouldn't create an entity but would only update existing entities. Create would be used when we know that an entity doesn't exist and getOrCreate is used mostly for convenience and typing because get isn't guaranteed.
There was a problem hiding this comment.
Another difference between create and update is that create would have default values while update would not have default values
There was a problem hiding this comment.
Hey @coreyar, the new getOrCreate methods handles all - creating, getting and updating internally and just calling it will give you an updated entity at any point. There is no loss of functionality.
// earlier
market = getMarket(id)
if (!market) market = createMarket(id)
updateMarket(market)
// is same as
market = getOrCreateMarket(id)
There was a problem hiding this comment.
I like the organization of the functions so that you had fine grained control over interactions with the subgraph.
For example, we want to use get in situations where an entity should be guaranteed and throw a critical error if it doesn't exist. Or explicitly create when we know it doesn't exist. The idea was to be more imperative in the interactions with the store. All other repos we reworked to follow this pattern as well.
If I was to revisit this I'd want to define a new approach and then update all the subgraphs to match it. My goal is to keep all of our subgraphs as consistent in style and patterns as possible.
| comptroller.closeFactor = comptrollerContract.closeFactorMantissa(); | ||
| comptroller.liquidationIncentive = comptrollerContract.liquidationIncentiveMantissa(); | ||
| comptroller.maxAssets = comptrollerContract.maxAssets(); | ||
| const marketId = marketAddress.toHexString(); |
There was a problem hiding this comment.
nit: Could use the market getter here
| import { nullAddress } from '../constants/addresses'; | ||
|
|
||
| // checks if a call reverted, in case it is we return -1 to indicate the wanted value is not available | ||
| export function valueOrNotAvailableIntIfReverted( |
There was a problem hiding this comment.
nit: organize in separate files like other subgraphs
|
@dhruv-chauhan Great work on this! Thank you for contributing. I pulled your changes into another branch so I could open a PR and run some tests. I made a couple more commits for consistency with the other subgraph repos and fixed 1 failing test. Thanks again! 🙌 Edit: Changes were added to #138 |
The PR changes:
Dev deployment: https://thegraph.com/hosted-service/subgraph/dhruv-chauhan/venus-subgraph-2
*(though the dev deployment is still syncing, checking initial data with the official subgraph looks good)