-
Notifications
You must be signed in to change notification settings - Fork 86
[Fix] get fresh node prices for paid endpoint calls #860
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
[Fix] get fresh node prices for paid endpoint calls #860
Conversation
…s in pricingContext and enhance logging in priceFeedApi
…ns to prevent CI formatting failures
… git issues - Format README.md to fix code style issues detected by CI - Change NX defaultBase to origin/master for better CI compatibility - Replace nx format:check with npx prettier to avoid git branch dependencies - Add auto-formatting with clear failure messages
- Remove exit 1 after successful auto-formatting - CI will now pass if files can be auto-formatted successfully - Add informational message about auto-formatted changes
packages/networks/src/networks/vNaga/envs/naga-dev/naga-dev.module.ts
Outdated
Show resolved
Hide resolved
packages/networks/src/networks/vNaga/envs/naga-local/naga-local.module.ts
Outdated
Show resolved
Hide resolved
// 1. Generate a key set for the JIT context | ||
const keySet: KeySet = {}; | ||
|
||
for (const url of connectionInfo.bootstrapUrls) { |
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 we have this functions in one single place instead of one per network? It will be impossible to maintain this and a slight difference like this comment on a different line will interfere with the IDE telling us the code is in multiple places
Also this makes CRing here in GH infeasible, how can I know there are other places that need this change? This will very quickly lead to impossible to fix bugs across different networks if not 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.
re: this functions in one single place instead of one per network?
The issue is that there are still some changes being made to the naga branch (eg, this convo) on the lit-assets, IMO. is not concrete enough yet to create a single shared interface (with an option to override logics) to be implemented. (Of course there are things we can start doing, just not the highest priority yet)
We should definitely handle this in a follow up PR - I have another ticket that I have to sync up with the latest changes on the lit-assets naga develop
branch so would be a good place to start
Also, opened a discussion ticket here:
https://linear.app/litprotocol/issue/JSS-37/naga-sdk-unify-shared-logic-with-an-option-to-override-for-all-network
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.
The major differences between the Naga networks is very similar to the Datil- attestations & payment for non-dev networks. @Ansonhkg
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 now resolved in this PR
#864
// 2. Fetch the price feed info | ||
const nodePrices = await getNodePrices( | ||
{ | ||
realmId: 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.
Do we need to pass the realmId
? If it is hardcoded then let's use a constant in a single place, get it from the chain as it could change, or hardcode it in the request call
This way it will be very complex to find and change when that needs a real 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.
yea we do - I think we should handle all these with a separate PR for shared helpers and logics etc. This value is ideally be dealt within a network environment based helper.
…itClient and chain manager files
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.
Pull Request Overview
This PR updates the node pricing mechanism for paid endpoint calls by replacing static price information with dynamically fetched node prices. The implementation introduces a caching mechanism with intelligent fetching logic to improve performance and ensure fresh pricing data.
- Integrates the existing
getNodePrices
function with caching and deduplication logic into the JIT context creation - Replaces static
currentConnectionInfo.priceFeedInfo.networkPrices
with dynamically fetchedjitContext.nodePrices
across all signing operations - Consolidates hardcoded development private keys into a single constant for consistency
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
packages/types/src/lib/v2types.ts | Adds NodePrices type definition and includes it in NagaJitContext |
packages/networks/src/networks/vNaga/envs//naga-.module.ts | Updates JIT context creation to fetch fresh node prices using getNodePrices |
packages/networks/src/networks/vNaga/LitChainClient/apis/highLevelApis/priceFeed/priceFeedApi.ts | Enhances price feed API with logging and improved type definitions |
packages/lit-client/src/lib/LitClient/createLitClient.ts | Updates signing operations to use dynamic node prices from JIT context |
packages/constants/src/lib/constants/constants.ts | Adds centralized DEV_PRIVATE_KEY constant |
Multiple chain manager files | Replaces hardcoded private keys with the centralized constant |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -495,7 +500,21 @@ const networkModuleObject = { | |||
}; | |||
} | |||
|
|||
return { keySet }; | |||
// Use read-only account for viewing PKPs |
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.
The comment 'Use read-only account for viewing PKPs' is misleading since this account is being used to fetch node prices, not view PKPs. Consider updating the comment to reflect the actual purpose: 'Use read-only account for fetching node prices'.
// Use read-only account for viewing PKPs | |
// Use read-only account for fetching node prices |
Copilot uses AI. Check for mistakes.
@@ -495,7 +500,21 @@ const networkModuleObject = { | |||
}; | |||
} | |||
|
|||
return { keySet }; | |||
// Use read-only account for viewing PKPs |
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.
The comment 'Use read-only account for viewing PKPs' is misleading since this account is being used to fetch node prices, not view PKPs. Consider updating the comment to reflect the actual purpose: 'Use read-only account for fetching node prices'.
// Use read-only account for viewing PKPs | |
// Use read-only account for fetching node prices |
Copilot uses AI. Check for mistakes.
@@ -494,7 +499,22 @@ const networkModuleObject = { | |||
secretKey: nacl.box.keyPair().secretKey, | |||
}; | |||
} | |||
return { keySet }; | |||
|
|||
// Use read-only account for viewing PKPs |
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.
The comment 'Use read-only account for viewing PKPs' is misleading since this account is being used to fetch node prices, not view PKPs. Consider updating the comment to reflect the actual purpose: 'Use read-only account for fetching node prices'.
// Use read-only account for viewing PKPs | |
// Use read-only account for fetching node prices |
Copilot uses AI. Check for mistakes.
@@ -495,7 +500,21 @@ const networkModuleObject = { | |||
}; | |||
} | |||
|
|||
return { keySet }; | |||
// Use read-only account for viewing PKPs |
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.
The comment 'Use read-only account for viewing PKPs' is misleading since this account is being used to fetch node prices, not view PKPs. Consider updating the comment to reflect the actual purpose: 'Use read-only account for fetching node prices'.
// Use read-only account for viewing PKPs | |
// Use read-only account for fetching node prices |
Copilot uses AI. Check for mistakes.
@@ -1428,3 +1428,6 @@ export const SIWE_URI_PREFIX = { | |||
|
|||
export type SIWE_URI_PREFIX_TYPE = ConstantKeys<typeof SIWE_URI_PREFIX>; | |||
export type SIWE_URI_PREFIX_VALUES = ConstantValues<typeof SIWE_URI_PREFIX>; | |||
|
|||
|
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.
The DEV_PRIVATE_KEY constant should include a comment explaining its purpose and that it's the standard Anvil/Hardhat development private key to avoid confusion about security implications.
/** | |
* Standard Anvil/Hardhat development private key. | |
* This key is used for local development and testing only. | |
* It is publicly known and should never be used in production environments. | |
*/ |
Copilot uses AI. Check for mistakes.
@@ -1,7 +1,7 @@ | |||
{ | |||
"$schema": "./node_modules/nx/schemas/nx-schema.json", | |||
"affected": { | |||
"defaultBase": "main" | |||
"defaultBase": "origin/master" |
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.
Changing the default base from 'main' to 'origin/master' may cause issues if the repository's primary branch is actually 'main'. Verify that 'master' is the correct primary branch name for this repository.
"defaultBase": "origin/master" | |
"defaultBase": "main" |
Copilot uses AI. Check for mistakes.
WHAT
re: we need to call a method to get the current node prices so that we can use those prices on the sessionSigs
There are two
getMaxPricesForNodeProduct
functions, one is async and another one isn’t.The issue is that the current implementation in the V8 SDK only uses the helper function without the "intelligent" logic, and the nodePrices is coming from the currentConnectionInfo, the fix here is to replace that line of code to
getNodePrices
which will internally manage the fetching and caching etc.Behavioural Tests
Manual log inspection was completed to verify the expected caching behaviour.
Test 1 - Make two concurrent calls to trigger "Local promise is already fetching"
Test 2 - Testing stale cache by first execute within the cache window (3 seconds) and one beyond cache window of 3 seconds