-
Notifications
You must be signed in to change notification settings - Fork 17
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
556 add prices table to indexeddb #764
Conversation
612b0e0
to
7e2ddeb
Compare
packages/constants/src/assets.ts
Outdated
export const NUMERAIRE_TOKEN = 'test_usd'; | ||
export const NUMERAIRE_TOKEN_ID = localAssets.find( | ||
metadata => metadata.display === NUMERAIRE_TOKEN, | ||
)!.penumbraAssetId!; | ||
|
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.
In the future we can easily change the NUMERAIRE_TOKEN
if necessary
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.
See apps/extension/.env
. We should try to use env variables for this given mainnet/testnet will have a different numeraire. Looks like we started this already with USDC_ASSET_ID
. We should update or replace that.
let numerairePerUnit = 0; | ||
let pricedAsset: AssetId | undefined = undefined; | ||
|
||
// case for trading pair <pricedAsset,numéraire> |
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.
We cannot know in advance the order of assets in a trading pair, so we must handle both cases
const calculatePrice = (delta: Amount, unfilled: Amount, lambda: Amount): number => { | ||
const filledAmount = subtractAmounts(delta, unfilled); | ||
return isZero(delta) || isZero(lambda) || isZero(filledAmount) | ||
? 0 | ||
: divideAmounts(lambda, filledAmount).toNumber(); | ||
}; |
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.
https://discord.com/channels/824484045370818580/938328380917559326/1217859679057870858 <- price calculation discussions here
packages/types/src/indexed-db.ts
Outdated
@@ -171,6 +178,10 @@ export interface PenumbraDb extends DBSchema { | |||
key: string; // bech32-encoded validator identity key | |||
value: Jsonified<ValidatorInfo>; | |||
}; | |||
PRICES: { | |||
key: string[]; // composite key [base64 EstimatedPrice['priced_asset'][inner'], base64 EstimatedPrice['numeraire']['inner']] |
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.
Using the composite key ensures that we will not have two records for the combination priced_asset:numeraire
and will always store only the most recent value
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.
You can also do:
key: [
Jsonified<Required<EstimatedPrice>['pricedAsset']['inner']>,
Jsonified<Required<EstimatedPrice>['numeraire']['inner']>,
];
8b85b79
to
7a10bda
Compare
packages/constants/src/assets.ts
Outdated
export const NUMERAIRE_TOKEN = 'test_usd'; | ||
export const NUMERAIRE_TOKEN_ID = localAssets.find( | ||
metadata => metadata.display === NUMERAIRE_TOKEN, | ||
)!.penumbraAssetId!; | ||
|
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.
See apps/extension/.env
. We should try to use env variables for this given mainnet/testnet will have a different numeraire. Looks like we started this already with USDC_ASSET_ID
. We should update or replace that.
if (compactBlock.swapOutputs.length) | ||
await this.updatePrices(compactBlock.swapOutputs, compactBlock.height); |
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.
Let's add brackets here
@@ -388,4 +409,46 @@ export class BlockProcessor implements BlockProcessorInterface { | |||
await this.indexedDb.upsertValidatorInfo(validatorInfoResponse.validatorInfo); | |||
} | |||
} | |||
|
|||
private async updatePrices(swapOutputs: BatchSwapOutputData[], height: bigint) { |
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.
Can you add documentation (here or calculatePrice()
) on some of these terms like delta/lambda/unfilled means?
|
||
await this.indexedDb.updatePrice(pricedAsset, NUMERAIRE_TOKEN_ID, numerairePerUnit, height); | ||
} | ||
} |
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 worth adding some tests for this. Some ideas:
-
We move updatePrices into its own file as a separate function (not on the class). This would require passing in the deps (like storage) into the function. When separated, we can write tests.
-
If not, just start creating a test suite
block-processor.test.ts
passing in deps on the class constructor.
}); | ||
|
||
await this.u.update({ | ||
table: 'PRICES', |
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 missing the key
right? or is it automatic?
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 works automatically if the key is specified via keypath
packages/types/src/indexed-db.ts
Outdated
@@ -171,6 +178,10 @@ export interface PenumbraDb extends DBSchema { | |||
key: string; // bech32-encoded validator identity key | |||
value: Jsonified<ValidatorInfo>; | |||
}; | |||
PRICES: { | |||
key: string[]; // composite key [base64 EstimatedPrice['priced_asset'][inner'], base64 EstimatedPrice['numeraire']['inner']] |
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.
You can also do:
key: [
Jsonified<Required<EstimatedPrice>['pricedAsset']['inner']>,
Jsonified<Required<EstimatedPrice>['numeraire']['inner']>,
];
7a10bda
to
a7a91cd
Compare
c8a4dae
to
6d6ebac
Compare
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.
Nice job with those tests. Think this is fine for now, but we should have a follow up PR enabling multiple numeraires.
packages/services/src/index.ts
Outdated
@@ -110,6 +115,7 @@ export class Services implements ServicesInterface { | |||
viewServer, | |||
querier: this.querier, | |||
indexedDb, | |||
numeraireAssetId: numeraireAssetId, |
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.
Think you can do:
- numeraireAssetId: numeraireAssetId,
+ numeraireAssetId,
partially close #556