-
Notifications
You must be signed in to change notification settings - Fork 6
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
Subgraph: Exposing data to populate Activity Details for stake operation #330
Conversation
✅ Deploy Preview for acre-dapp-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
49d7f36
to
79ffcbd
Compare
a1df225
to
fbfc805
Compare
fbfc805
to
8d44058
Compare
subgraph/subgraph.yaml
Outdated
indexerHints: | ||
prune: auto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we remove it? Should we keep it or is it unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because indexerHints
are not supported prior to 1.0.0. It's throwing an error while deploying locally:
Failed to deploy to Graph node http://localhost:8020/: subgraph resolve error: resolve error:
indexerHints are not supported prior to 1.0.0
.
We can leave it, but we need to increase the specVersion
prop to min. ver: 1.0.0: 742d770
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should set specVersion
to the latest to support all features? Ofc non-blocking, just wondering.
f8dd76c
to
7a58df6
Compare
1ab8386
to
497cdae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'm leaving it for @r-czajkowski for final approval.
test("Deposit entity should exist", () => { | ||
assert.entityCount("Deposit", 1) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we test a case when the Deposit
entity doesn't exist ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't we throw an error as we did before? We can test expected errors in subgraph https://thegraph.com/docs/en/developing/unit-testing-framework/#expected-failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throw an error when the Deposit entity deosn't exist. We throw an error just to improve readability of the code. This should never happen because the graph indexes events chronologically and the deposit can only be finalized once it has been initialized. This case is possible when we set the wrong `startBlock` in manifest but we always set the `startBlock` to block where a given contract was deployed. Cover this case in unit tests.
We changed the type of the `referral` field to `Int` in 8197d98 so just for consistency we update it in tests as well.
Subgraph: Activity Details for deposit operation
What was done
handleDepositInitialized
eventhandleDepositFinalized
eventDepositOwner
,Deposit
,Withdraw
,Event
] & interfaceActivityData
Preview
Additional info
DEVELOPMENT QUERY URL - LATEST VERSION:
https://api.studio.thegraph.com/query/69074/a-test/version/latest