feat: Add Bitcoin indexing #215
Conversation
mj52951
requested review from
wainola,
tcar121293,
mpetrun5,
MakMuftic and
nmlinaric
| import { Block } from "./bitcoinTypes" | ||
|
|
||
| const BLOCK_TIME = Number(process.env.BLOCK_TIME) || 12000 | ||
| const BLOCK_DELAY = Number(process.env.BLOCK_DELAY) || 3 |
There was a problem hiding this comment.
I would prefer if these are config options that can be passed to the constructor.
There was a problem hiding this comment.
Problem with this whole approach to that being one env variable is that evm, substrate and btc all have different BLOCK_TIME and BLOCK_DELAY default options. And e.g. setting one value as block delay for all three network types doesn't look correct from my perspective. So what do you think of adding 4 more different env variables as BLOCK_DELAY_EVM, BLOCK_DELAY_SUBSTRATE and similar?
There was a problem hiding this comment.
Different networks have different params in this case also.
One EVM network won't have the same params as another.
Setting it by domainID_variableName would fix that.
| } | ||
| } | ||
|
|
||
| function decodeDepositEvent(tx: Transaction, domain: Domain): DecodedDeposit | undefined { |
There was a problem hiding this comment.
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.
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.
Add some description of where and why it happened.
There was a problem hiding this comment.
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.
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.
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.
If it is a big refactor I would avoid it/do it separetly for now.
There was a problem hiding this comment.
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.
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.
I think it's better to do it in a separate PR. Along with refactoring saveDeposit functions.
mj52951
force-pushed
the
mj52951/bitcoin-indexing
branch
from
58af62e
to
49831c2
Compare
Added Bitcoin indexing functionality.
closes #201
closes #202
closes #203