-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: Add Bitcoin indexing #215
base: main
Are you sure you want to change the base?
Conversation
} | ||
} | ||
|
||
function decodeDepositEvent(tx: Transaction, domain: Domain): DecodedDeposit | undefined { |
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 think it is more intuitive to throw errors for unexpected behaviours instead of returning undefined and then we don't what happened. Also, would remove the need to have decoding and then undefined checks in saveEvents
Probably decoding errors will need to be caught and handled appropriately in saveEvents
and logged as warn
as we most likely don't want to try decoding that again, but at least we will know what happened.
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.
Hm, I might've made a bad approach for the whole workflow then. But what this is essentially doing is checking if the output address is the relayer address (resourceFound
) and if it is, it parses and returns the data so it can be stored in the database. If it isn't, it returns undefined
. So it'll return undefined for every transaction that isn't actually a Sygma deposit.
try { | ||
amountInUSD = await coinMakerCapService.getValueInUSD(decodedDeposit.amount.toString(), decodedDeposit.resource.symbol) | ||
} catch (error) { | ||
logger.error((error as Error).message) |
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.
Add some description of where and why it happened.
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.
Coinmarketcap service functions already log the whole URL that was unsuccessfully fetched and additionally throw the message Error getting value from CoinMarketCap
which is shown to be caught and logged here so not sure should I provide any more context?
} | ||
} | ||
|
||
async function saveDeposit( |
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 feel like we have this logic somewhere else as well.
See if it can be reused instead of copied.
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.
Yes, something like getUpdatedTransfer function was in the squid-indexer
repo. Just, that would require a partial refactor of evm and substrate parts also, so not sure should I do it in this PR ?
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.
If it is a big refactor I would avoid it/do it separetly for now.
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 think it's better to do it in a separate PR. Along with refactoring saveProposalExecution
functions.
return executionData | ||
} | ||
|
||
async function saveProposalExecution( |
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.
A lot of this save functions have basically the same logic as other networks.
Would be prudent to check if it can be unified.
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 think it's better to do it in a separate PR. Along with refactoring saveDeposit
functions.
58af62e
to
49831c2
Compare
Added Bitcoin indexing functionality.
closes #201
closes #202
closes #203