-
Notifications
You must be signed in to change notification settings - Fork 234
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
fix (#patch); euler-finance; fix oneshot cancelled error #1174
Conversation
The fix solves the "oneshot cancelled" error, but the revenues are wrong without fixing the implementation to get interest revenue from AssetStatus event as discussed in #1139 |
89e62a5
to
73694e7
Compare
52dc9b3
to
43a3758
Compare
lint issues will be fixed in a separate PR |
It is finally working: https://thegraph.com/hosted-service/subgraph/tnkrxyz/euler?version=pending&selected=logs (with grafting) and https://thegraph.com/hosted-service/subgraph/tnkrxyz/test-subgraph?version=pending&selected=logs (fresh deployment without grafting) |
const totalBorrows = event.params.totalBorrows; | ||
const reserveBalance = event.params.reserveBalance; | ||
const interestAccumulator = event.params.interestAccumulator; | ||
//const interstRate = event.params.interestRate; |
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.
remove this line
} | ||
|
||
export function handleGovSetAssetConfig(event: GovSetAssetConfig): void { | ||
updateLendingFactors(event); | ||
updateLendingFactors(event); |
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.
this is fine, but generally you don't want to make formatting changes to lines of code outside of the scope of the PR.
This will be better when we have a standard format across the whole repo, but something to keep in mind.
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.
Mainly just because it makes the PR more messy
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.
Good point. I use prettier addon for vscode to automatically format the file when I save. I will revert the format changes.
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.
No need to revert if it gets too messy
|
||
if (protocolUtility.lastBlockNumber >= blockNumber - 120 ) { | ||
/* |
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.
Seems like there is still a lot of cleaning up of comments before it is ready to merge
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.
I will remove the commented lines
const token = getOrCreateToken(underlying); | ||
const assetStatus = getOrCreateAssetStatus(marketId); | ||
|
||
assert( |
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 are you using asserts here?
I think we should stick with the error checking patterns that we use throughout the repo
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.
Will fix
let protocolUtility = getOrCreateProtocolUtility(blockNumber); | ||
let markets = protocolUtility.markets; | ||
const marketIndex = markets.indexOf(marketId); | ||
assert(marketIndex >= 0, `[updateProtocolAndMarkets]marketId=${marketId} not in protocolUtility.markets`); |
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.
same here
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.
Will fix
@dmelotik based on our discussion on discord, I will commit the minimal fix in a separate PR (and test it with even small chunk size). We can assess which one to merge. |
The deployment of the minimal fix with even smaller chunk size (2 markets) still failed. And it makes me think I probably mis-diagnosed the root cause. The problem is still within the loop, but one of the markets (specifically, 0xc00e94cb662c3520282e6f5717214004a7f26888) is causing the error at block 15700199, instead of the size of the loop. I will hold off moving forward with this PR and hope to figure out why the market is causing the error and fix the problem there. |
Close this PR as it is replaced by #1213 |
This is a quick fix for the error from euler finance subgraph deployment on recent blocks:
and the underlying error msg
Ready to review/merge if verified working https://thegraph.com/hosted-service/subgraph/tnkrxyz/euler?version=pending&selected=logs