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

Liquity Subgraph v1 #8

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Liquity Subgraph v1 #8

wants to merge 6 commits into from

Conversation

steve0xp
Copy link

@steve0xp steve0xp commented Apr 10, 2022

The intent of this PR is to create and deploy the first iteration of the liquity subgraph that follows the "Lending Subgraph Standard" schema and general intent. The subgraph has been deployed to the hosted service and can be found here.

This PR is just the first draft and further testing, design iterations, and reformatting need to be carried out. There are some issues being seen as well in a couple of entities when querying the subgraph, but the work done so far needed to be reviewed now.

As well, please note that I was having issues with using helper functions and, for the sake of time, wrote the mappings within mainly two large event handlers. This would, of course, be reformatted in future iterations. The data also needs to be confirmed by comparing to other reputable data sources still.

Notes and TODOs are outlined within the README, and there are related TODO's commented within the respective mapping files.

NOTE: rebasing and commit squashing may be needed before merging to main since there are "WIP commits" and I had accidentally run yarn prettier-write on the entire repo which edited the some of the AAVE and possibly other protocol-related subgraph files

@steve0xp steve0xp requested a review from davekaj April 10, 2022 03:02
Copy link
Collaborator

@davekaj davekaj left a comment

Choose a reason for hiding this comment

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

most the change requests are small. overall the PR is good! it's quite straight forward.

The one thing I am worried about is that you said some of the entities are not showing up, or null, or something like that. we would have to fix that before merging this PR.

we should move off of this for now, just read the comments and don't worry about fixing them for now. I just wanted to see how the PR looked. Overall it's good. The one problem is those missing and/or null entities

@@ -0,0 +1,5 @@
{
"network": "ethereum",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is an aave file, looks like you copied it over by accident

@@ -0,0 +1,5 @@
{
"network": "polygon",
"LendingPoolAddressesProviderRegistryAddress": "0x3ac4e9aa29940770aeC38fe853a4bbabb2dA9C19",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is an aave file, looks like you copied it over by accident

}

// AssemblyScript can't tell we will never reach this, so it insists on a return statement
return 'unreached'
Copy link
Collaborator

Choose a reason for hiding this comment

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

its easier to just make one of the three options the default of the switch caes

Comment on lines +10 to +12
export let ICR_min = BigDecimal.fromString('1.1')
export let TCR_min = BigDecimal.fromString('1.5')
export let EthToLUSD = BigDecimal.fromString('0.9')
Copy link
Collaborator

Choose a reason for hiding this comment

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

this kind of stuff should be read from the smart contracts. because if governance votes to change these, the contract breaks.

however i know they have low governance, so maybe these can never be changed. still, i would try to read it from the contract

Comment on lines +19 to +21
export const LUSDAddr = '0x5f98805A4E8be255a32880FDeC7F6728C6568bA0'

export const LiquityBorrowerOpsAddr = '0x24179CD81c9e782A4096035f7eC97fB8B783e007'
Copy link
Collaborator

Choose a reason for hiding this comment

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

these can likely also be read from the contracts

.concat('-')
.concat(event.transactionLogIndex.toString())
.concat('-')
.concat('1')
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this 1 for?

.concat('-')
.concat(event.transactionLogIndex.toString())
.concat('-')
.concat('2')
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this 2 for?

.div(exponentToBigDecimal(asset.decimals))
.truncate(asset.decimals)

if (operation == 'openTrove') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should do a if else chain with this, and the other two operations. its cleaner.

pseudo code:

if (open trove) 
  else if (closed trove)
   else (adjusted trove)

let protocol = getProtocol(market.protocol)
let asset = getOrCreateLUSD(market.collateralBackedAsset)
let account = getOrCreateAccount(event.params._borrower.toHexString())
let operation = getTroveOperationFromBorrowerOperation(event.params._operation)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this variable is never used... maybe it can be deleted?

Comment on lines +73 to +74
eventEntry.xTokenAmount = liquidatedCollateralAmount // TODO: not sure if I should have this in here or not in accordance to schema
eventEntry.amount = LiquidateAmount
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks weird to me, i think there is probably a better way to do it

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.

2 participants