-
Notifications
You must be signed in to change notification settings - Fork 20
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
add carp funnel with stake delegation pool tracking #246
Conversation
081aeba
to
a1831ef
Compare
9b76490
to
2a9f066
Compare
); | ||
return allData; | ||
} | ||
|
||
async function getSpecificCdeData( | ||
extension: ChainDataExtension, | ||
extension: ChainDataExtension & { startBlockHeight: number }, |
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.
What was the need to add this? It's not used anywhere in the function
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 don't understand this question, it's used in the first line of the function actually
import { query, getErrorResponse } from '@dcspark/carp-client/client/src/index'; | ||
import { Routes } from '@dcspark/carp-client/shared/routes'; | ||
|
||
const confirmationDepth = '10'; |
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.
Should have a comment rationale for why 10
was picked. If this isn't an ENV, it should be mentioned in the docs
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 made it an env... Since I think it should be configurable, although it's a bit problematic since it shouldn't really be changed without resyncing.
079776d
to
5ead855
Compare
just to test until the up to date version is published on npm
6080ca7
to
879218d
Compare
@@ -1,16 +1,7 @@ | |||
/* tslint:disable */ |
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 file is automatically generated and shouldn't be checked in. The reason this file changed is because sometimes it gets modified by automatic code formatting and I can't figure out why sometimes the rule to ignore this file for formatting works and sometimes it doesn't 😕
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.
deleted
if (!ENV.CARP_URL) { | ||
delete presyncBlockHeight[Network.CARDANO]; | ||
} |
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.
Could this not cause a subtle issue where if you specify a Cardano CDE but the CARP_URL is missing that it just silently doesn't track anything? I feel if you're using task that requires Carp and you didn't specify the CARP_URL it should be an error
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.
yeah, makes sense, changed it for an error
just to reduce the number of rpc calls
the cast is probably clearer than the alternatives
); | ||
|
||
let grouped = groupCdeData( | ||
Network.EVM, |
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 not cardano?
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.
well... It doesn't actually matter, I think.
The discriminant was introduced so that during the presync we can update the cardano cde tracking table instead of the other one, but during the normal sync just knowing the EVM block gives you the slot, so the field is not used.
But I guess we should put cardano here anyway to be consistent.
how do I get pool id to put in the config in |
The |
const startBlockheights = config.map(cde => cde.startBlockHeight).filter(sbh => !!sbh); | ||
const startBlockheights = config | ||
.map(cde => { | ||
if (cde.cdeType != ChainDataExtensionType.CardanoPool) { | ||
return cde.startBlockHeight; | ||
} else { | ||
return null; | ||
} | ||
}) | ||
.filter(sbh => !!sbh) as number[]; | ||
const minStartBlockheight = Math.min(...startBlockheights); | ||
return isFinite(minStartBlockheight) ? minStartBlockheight : -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.
All of this should probably be a single .reduce
call instead (same with the function below)
TODO
Example settings
Currently I'm setting things up like this:
extensions.yml
.env (example)
Ideally the carp setting should be a single one, but I needed this to make it work with the batcher running in docker.
Parsing
I had to setup a parsing rule like this:
(the lenghts are not tight bounds, just as an example for testing)