add distribution code #105
Conversation
WalkthroughThe changes introduce a reward distribution system based on Merkle trees. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ParifiSdk
participant RewardDistribution
participant MerkleTree
participant RewardDistributorContract
User->>ParifiSdk: claimRewards({ user, rounds, tokenSymbol })
ParifiSdk->>RewardDistribution: claimRewards(params)
RewardDistribution->>RewardDistribution: getRewardContract(tokenSymbol)
RewardDistribution->>RewardDistribution: getTree(tokenSymbol, round)
RewardDistribution->>MerkleTree: Build Merkle tree from rewards
RewardDistribution->>RewardDistribution: getUserData(user, tokenSymbol, round)
RewardDistribution->>RewardDistribution: Generate proof for user
RewardDistribution->>RewardDistributorContract: Prepare claimReward transaction
RewardDistribution-->>ParifiSdk: Return transactions and amounts
ParifiSdk-->>User: Return claim transactions
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(2 hunks)src/common/distributionData.ts(1 hunks)src/distribution/index.ts(1 hunks)src/index.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/index.ts (1)
src/distribution/index.ts (1)
RewardDistribution(10-130)
src/distribution/index.ts (1)
src/common/distributionData.ts (3)
REWARD_DISTRIBUTOR_ADDRESSES(26-31)REWARD_DISTRIBUTOR_ABI(33-218)USERS_REWARDS(11-22)
🔇 Additional comments (11)
package.json (2)
49-49: LGTM: Added OpenZeppelin Merkle Tree dependencyAdding the
@openzeppelin/merkle-treedependency is appropriate for implementing the Merkle tree functionality needed in the new reward distribution feature.
59-59:✅ Verification successful
Consider security implications of relaxing version constraints
Changing from a fixed version (
"2.17.1") to a caret range ("^2.17.1") allows automatic updates of minor and patch versions, which could potentially introduce breaking changes despite semver promises.
🏁 Script executed:
#!/bin/bash # Check the changelog of viem package to assess risks of automatic minor version updates npm view viem versions --json | jq -r 'map(select(startswith("2.17") or startswith("2.18") or startswith("2.19") or startswith("2.20")))[]' | sort -V curl -s https://api.github.com/repos/wevm/viem/releases | jq -r '.[] | select(.tag_name | startswith("v2.17") or startswith("v2.18") or startswith("v2.19") or startswith("v2.20")) | {tag_name: .tag_name, published_at: .published_at, body: .body}' | head -n 20Length of output: 625
🏁 Script executed:
#!/usr/bin/env bash set -euo pipefail # Resolve the true GitHub repository for viem repo_url=$(npm view viem repository.url | sed -e 's/^git+//' -e 's/\.git$//') owner_repo=${repo_url#https://github.com/} echo "Scanning releases for $owner_repo …" # Fetch releases for 2.17.x through 2.20.x and flag any breaking changes curl -s "https://api.github.com/repos/$owner_repo/releases" | jq -r ' .[] | select(.tag_name | test("^v2\\.(17|18|19|20)")) | { tag: .tag_name, published_at, prerelease, hasBreaking: (.body // "" | test("BREAKING CHANGE")) } 'Length of output: 488
Caret range update is safe—no breaking changes detected
I reviewed the viem changelog and GitHub releases for v2.17.x–v2.20.x and found no documented “BREAKING CHANGE” entries in any minor or patch release. Updating the constraint to
"^2.17.1"is compatible with semver.• File: package.json (line 59)
• Dependency:"viem": "^2.17.1"Ensure your lockfile is committed and CI tests cover dependency upgrades to catch any unforeseen issues.
src/index.ts (3)
7-7: LGTM: Clean import of the new RewardDistribution classThe import statement is correctly defined to include the new reward distribution functionality.
24-24: LGTM: Added public property for reward distributionAdding
rewardDistributionas a public property makes the new functionality available to SDK consumers.
39-39: LGTM: Proper initialization of the reward distribution instanceThe
RewardDistributioninstance is correctly initialized in the constructor without parameters.src/common/distributionData.ts (2)
5-7: LGTM: Created enum for supported chainsThe
SUPPORTED_CHAINSenum is a good practice for managing chain identifiers.
33-217: LGTM: Well-defined contract ABIThe ABI is comprehensive and includes all necessary functions, events, and error types for interacting with the reward distributor contract.
src/distribution/index.ts (4)
1-8: LGTM: Good imports and proper organizationThe imports are appropriately organized, bringing in the necessary dependencies for Merkle tree operations and contract interactions.
16-29: LGTM: Good error handling in getRewardContract methodThe method properly validates the existence of a reward contract for the given token symbol and throws a descriptive error if none is found.
37-51: LGTM: Proper Merkle tree constructionThe
getTreemethod correctly builds a StandardMerkleTree with the appropriate format and throws an error when no rewards are found.
85-97: LGTM: Robust multi-round reward claimingThe
claimRewardsmethod handles errors for individual rounds without failing the entire batch, which is a good practice for resilience.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/common/constants.ts (2)
97-110: Consider a dynamic approach for user rewards.The commented code suggests you plan to fetch user rewards from a backend service. This is preferable to hardcoding values in the SDK, as it would avoid needing to publish new versions with each rewards update.
You should document this approach in a more formal comment if you intend to implement it in the future.
189-199: Ensure proper documentation for the claim function.The
claimRewardfunction is a critical part of this distribution system, requiring users to provide proof for their rewards.Consider adding developer documentation (as comments in the SDK) explaining how to generate the Merkle proofs needed for this function. This would make the SDK more developer-friendly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/common/constants.ts(2 hunks)src/distribution/index.ts(1 hunks)src/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/index.ts
- src/distribution/index.ts
🔇 Additional comments (5)
src/common/constants.ts (5)
2-2: Good addition of type safety for addresses.Adding the
Addressimport fromviemensures proper typing for Ethereum addresses, which is essential for type safety in TypeScript.
93-95: Using enum for chain IDs is a good practice.The
SUPPORTED_CHAINSenum provides type safety and readability for chain identification in the codebase.
121-306: The reward distributor ABI is comprehensive and well-structured.The ABI includes all necessary components:
- Constructor and error definitions
- Event declarations for important state changes
- Functions for claiming rewards (single and batch)
- Role management functions (AccessControl pattern)
- Round management functions
The implementation follows best practices for smart contract interfaces.
200-210: Batch claiming functionality is a good optimization.The inclusion of
claimRewardsfor batch processing is an excellent optimization that will save gas costs for users with multiple unclaimed rewards across different rounds.
114-119:❓ Verification inconclusive
Verify the zero address for PRF token on BASE.
The PRF token address is set to the zero address (
0x0000000000000000000000000000000000000000) on the BASE chain.Is this intentional as a placeholder, or should there be a valid contract address? If it's a placeholder, consider adding a comment to clarify this.
🏁 Script executed:
#!/bin/bash # Check if there are any references to the PRF token address being set elsewhere rg -i "prf.*address" --type tsLength of output: 32
🏁 Script executed:
#!/bin/bash # Locate the file containing the REWARD_DISTRIBUTOR_ADDRESSES constant file=$(rg --files-with-matches "export const REWARD_DISTRIBUTOR_ADDRESSES" --type ts) echo "File: $file" # Print the declaration and following 30 lines to inspect all chain entries rg -n "export const REWARD_DISTRIBUTOR_ADDRESSES" -A 30 "$file"Length of output: 1389
Confirm PRF zero address on Base
The PRF token is currently set to the zero address (
0x0000000000000000000000000000000000000000) forSUPPORTED_CHAINS.BASE(src/common/constants.ts lines 114–119). No other references to a PRF address were found.• If this is a placeholder, please add a comment explaining that.
• If not, please replace it with the correct PRF contract address.
src/index.ts
Outdated
| @@ -28,6 +28,7 @@ export class ParifiSdk { | |||
| subgraphConfig: SubgraphConfig, | |||
| relayerConfig: RelayerConfig, | |||
| pythConfig: PythConfig, | |||
| rewardList: RewardList, | |||
There was a problem hiding this comment.
I'm still not sure if this is the proper approach to do this, as the sdk will need to be reinitialized every time we need the updated reward list. Ideally this should be a param to the functions.
However, as we need to roll this out asap, we can continue with this approach for now, and revisit this later.
This should be an optional config.
|
@racso2609 Please add try-catch for the functions to return 0 in case of any errors. And add comments |
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (7)
test/subgraph-tests/index.test.ts (1)
30-48: Added test for new rewards estimation functionalityGood test for the new rewards distribution functionality. The test covers basic functionality by verifying that rewards calculation returns a positive value.
Consider adding additional assertions to test edge cases:
- What happens with invalid addresses?
- What about zero rewards or periods with no activity?
- Is there a test for precise reward calculation with known data?
Also, consider implementing the try-catch blocks for error handling in the reward distribution functionality as requested in the PR comments.
test/subgraph-tests/orders.test.ts (1)
30-30: Remove console.log statementConsole.log statements should be removed from test code before merging to keep test output clean.
- console.log(orderCount);src/subgraph/scripts/subgraphQueries.ts (2)
8-31: Add documentation commentsAs requested in the PR comments, please add explanatory comments to improve code clarity. Include information about what the query retrieves and how it's used in the reward distribution system.
export const fetchIntegratorFeesWithTimestampQuery = ( startTimeStamp: number, // unix timestamp endTimeStamp: number, //unix timestamp ) => + /** + * Fetches SNX accounts and their settled orders within a specified time range + * Used for calculating reward distributions based on fees generated + * + * @returns GraphQL query to retrieve accounts and their settled orders with fee information + */ gql`{ snxAccounts( first: 1000 where: { type: PERP, totalOrdersCount_gt: 0 }) { id integratorFeesGenerated accountId owner { id } orders( where: { status: SETTLED, createdTimestamp_gt: ${startTimeStamp}, createdTimestamp_lt: ${endTimeStamp} } ) { id referralFees collectedFees } } } `;
10-11: Consider adding pagination supportThe query has a hard limit of 1000 records. For a robust implementation, consider adding pagination support to handle cases where there might be more than 1000 SNX accounts.
export const fetchIntegratorFeesWithTimestampQuery = ( startTimeStamp: number, // unix timestamp endTimeStamp: number, //unix timestamp + first: number = 1000, + skip: number = 0 ) => gql`{ snxAccounts( - first: 1000 + first: ${first} + skip: ${skip} where: { type: PERP, totalOrdersCount_gt: 0 }) { id integratorFeesGenerated accountId owner { id } orders( where: { status: SETTLED, createdTimestamp_gt: ${startTimeStamp}, createdTimestamp_lt: ${endTimeStamp} } ) { id referralFees collectedFees } } } `;src/subgraph/orders/index.ts (1)
43-69: Add try-catch block with error handling as requested in PRThe function correctly returns 0 on error as requested in the PR comments, but consider adding additional comments for better code clarity.
// Get count of settled orders by a user address export const getSettledOrdersCountBasedOnTimeStampByUserAddress = async ( subgraphEndpoint: string, userAddress: string, count = 50, skip = 0, startTimeStamp: number, endTimeStamp: number, ): Promise<number> => { try { const response: any = await request( subgraphEndpoint, fetchSettledOrdersCountBasedOnTimeStampByUserQuery(userAddress, count, skip, startTimeStamp, endTimeStamp), ); const snxAccounts = mapResponseToSnxAccountArray(response?.snxAccounts) ?? []; if (!snxAccounts.length) return 0; const orderCount = snxAccounts.reduce((total, account) => total + (account.orders?.length || 0), 0); return orderCount; } catch (error) { console.error('Error fetching orders:', error); return 0; } };src/subgraph/orders/subgraphQueries.ts (1)
56-81: Update the comment to correctly describe this query functionThe comment is incorrect as it's a copy from the previous function. It should describe this specific query which fetches settled orders count based on timestamp.
-// Fetch all orders by `userAddress` +// Fetch count of settled orders by `userAddress` within a specific time range export const fetchSettledOrdersCountBasedOnTimeStampByUserQuery = ( userAddress: string, count: number = 50, skip: number = 0, startTimeStamp: number, // unix timestamp endTimeStamp: number, //unix timestamp ) => gql`{ snxAccounts( first: ${count} skip: ${skip} where: { owner: "${userAddress}", type: PERP }) { id accountId owner { id } orders( where: {status: SETTLED, createdTimestamp_gt: ${startTimeStamp}, createdTimestamp_lt: ${endTimeStamp}} ) { id } } } `;src/subgraph/index.ts (1)
152-168: Add JSDoc comments to maintain code documentation consistencyConsider adding JSDoc comments to this method similar to the one added for
getEstimatedRewardsForUserto maintain consistency in code documentation.+ /** + * Gets the count of settled orders for a user within a specific time range + * + * @param userAddress - The address of the user to fetch orders for + * @param count - The maximum number of accounts to fetch (pagination) + * @param skip - The number of accounts to skip (pagination) + * @param startTime - The starting Unix timestamp of the period + * @param endTime - The ending Unix timestamp of the period + * @returns A Promise resolving to the count of settled orders + */ public async getSettledOrdersCountBasedOnTimeStampByUserAddress( userAddress: string, count: number, skip: number, startTime: number, endTime: number, ): Promise<number> { const subgraphEndpoint = this.getSubgraphEndpoint(this.rpcConfig.chainId); return await getSettledOrdersCountBasedOnTimeStampByUserAddress( subgraphEndpoint, userAddress, count, skip, startTime, endTime, ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
package.json(3 hunks)src/common/constants.ts(2 hunks)src/common/subgraphMapper.ts(5 hunks)src/distribution/index.ts(1 hunks)src/index.ts(3 hunks)src/interfaces/sdkTypes.ts(5 hunks)src/subgraph/accounts/index.ts(1 hunks)src/subgraph/accounts/subgraphQueries.ts(0 hunks)src/subgraph/index.ts(4 hunks)src/subgraph/orders/index.ts(2 hunks)src/subgraph/orders/subgraphQueries.ts(3 hunks)src/subgraph/positions/index.ts(2 hunks)src/subgraph/positions/subgraphQueries.ts(9 hunks)src/subgraph/protocol/index.ts(1 hunks)src/subgraph/scripts/index.ts(1 hunks)src/subgraph/scripts/subgraphQueries.ts(1 hunks)test/common/constants.ts(1 hunks)test/pyth-tests/pyth.test.ts(2 hunks)test/subgraph-tests/account.test.ts(1 hunks)test/subgraph-tests/index.test.ts(2 hunks)test/subgraph-tests/market.test.ts(1 hunks)test/subgraph-tests/orders.test.ts(1 hunks)test/subgraph-tests/portfolio.test.ts(0 hunks)test/subgraph-tests/position.test.ts(1 hunks)test/subgraph-tests/protocol.test.ts(0 hunks)
💤 Files with no reviewable changes (3)
- test/subgraph-tests/protocol.test.ts
- src/subgraph/accounts/subgraphQueries.ts
- test/subgraph-tests/portfolio.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/subgraph/protocol/index.ts
- test/subgraph-tests/account.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- src/common/constants.ts
- src/index.ts
- src/distribution/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/subgraph/positions/index.ts (1)
src/subgraph/positions/subgraphQueries.ts (1)
fetchOpenPositionsByUser(116-171)
test/subgraph-tests/position.test.ts (2)
test/index.ts (1)
getParifiSdkInstanceForTesting(5-33)test/common/constants.ts (2)
TEST_USER_1(8-8)TEST_USER_2(9-9)
test/subgraph-tests/orders.test.ts (2)
test/index.ts (1)
getParifiSdkInstanceForTesting(5-33)test/common/constants.ts (2)
TEST_USER_1(8-8)TEST_ORDER_ID1(2-2)
src/subgraph/orders/index.ts (3)
src/subgraph/index.ts (1)
getSettledOrdersCountBasedOnTimeStampByUserAddress(152-168)src/subgraph/orders/subgraphQueries.ts (1)
fetchSettledOrdersCountBasedOnTimeStampByUserQuery(57-81)src/common/subgraphMapper.ts (1)
mapResponseToSnxAccountArray(70-80)
src/subgraph/scripts/index.ts (3)
src/subgraph/index.ts (1)
getEstimatedRewardsForUser(315-329)src/subgraph/scripts/subgraphQueries.ts (1)
fetchIntegratorFeesWithTimestampQuery(4-31)src/common/constants.ts (1)
WAD(8-8)
src/subgraph/index.ts (2)
src/subgraph/orders/index.ts (1)
getSettledOrdersCountBasedOnTimeStampByUserAddress(44-69)src/subgraph/scripts/index.ts (1)
getEstimatedRewardsForUser(6-48)
🪛 Biome (1.9.4)
test/subgraph-tests/orders.test.ts
[error] 18-18: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
🔇 Additional comments (22)
test/subgraph-tests/market.test.ts (1)
8-8: More robust test assertionChanging from an exact number to a "greater than" assertion makes the test more resilient to market additions in the subgraph data while still validating that a substantial number of markets are returned.
src/subgraph/positions/index.ts (2)
6-6: LGTM: Added new importAdding the import for
fetchOpenPositionsByUserwhich is used in the updated implementation.
48-48: Enhanced open positions query implementationUpdated implementation to use the new
fetchOpenPositionsByUserquery which includes additional position data likesnapshotCollateralValueUsd.However, consider adding error handling with try-catch as requested in PR comments:
- const subgraphResponse: any = await request(subgraphEndpoint, fetchOpenPositionsByUser(userAddress, count, skip)); + try { + const subgraphResponse: any = await request(subgraphEndpoint, fetchOpenPositionsByUser(userAddress, count, skip)); + const snxAccounts = mapResponseToSnxAccountArray(subgraphResponse?.snxAccounts); + return snxAccounts ?? []; + } catch (error) { + console.error('Error fetching positions:', error); + return []; + }Wait, I just noticed that there is already a try-catch block around this code. This looks good then!
test/subgraph-tests/position.test.ts (4)
2-2: LGTM: Using constants for test usersGood practice to use constants from a central location rather than hardcoded values.
7-7: LGTM: Using standardized test user constantsReplaced hardcoded user addresses with constants from the shared constants file, improving maintainability.
Also applies to: 14-14
17-17: More flexible test assertionChanged from an exact number assertion to ensuring at least one position is returned, making the test more resilient to data changes.
36-45: LGTM: Code cleanup in testRemoved the
.onlymodifier that was preventing other tests from running and improved code structure.test/subgraph-tests/index.test.ts (1)
4-4: LGTM: Using test constantsAdded import for test user constant.
src/subgraph/accounts/index.ts (1)
12-12: Looks good: Import cleanup aligns with refactoringThe import statement has been updated to remove references to portfolio data types that are no longer used in this file. This change is consistent with the broader refactoring mentioned in the summary.
test/subgraph-tests/orders.test.ts (2)
2-2: LGTM: Improved test maintainability with constantsUsing constants from a shared file instead of hardcoded values improves maintainability and consistency across tests.
7-7: LGTM: Updated test cases to use constantsTests now properly reference shared constants instead of hardcoded values, which is a good practice for test maintainability.
Also applies to: 13-14
test/pyth-tests/pyth.test.ts (2)
6-6: LGTM: Simplified importsRemoving unused imports keeps the code clean and maintainable.
37-37: LGTM: Re-enabled previously skipped testRe-enabling this test improves test coverage for the authentication functionality.
src/subgraph/orders/subgraphQueries.ts (1)
47-47: Addition of snapshotCollateralValueUsd field looks goodThe field has been properly added to the query.
src/subgraph/index.ts (3)
21-21: Import update looks goodThe import statement has been properly updated to include the new function.
36-36: Import for new rewards function looks goodCorrectly importing the new function from the scripts module.
301-329: Great JSDoc documentation for this new methodThe JSDoc comments for this method are well-written and provide clear documentation for the function's purpose and parameters.
test/common/constants.ts (2)
8-16: LGTM, well-defined test user constantsThe addition of test user constants (TEST_USER_1 through TEST_USER_4) and their corresponding account IDs provides a clean, centralized way to reference these values throughout the test suite, which improves maintainability.
18-20: LGTM, clear naming of market constantAdding the comment "// ETH-USDC" provides helpful context for what this market ID represents.
src/subgraph/positions/subgraphQueries.ts (2)
51-51: LGTM, consistent field addition across queriesThe
snapshotCollateralValueUsdfield has been consistently added to all position-related queries, which is good for maintaining API consistency.Also applies to: 110-110, 168-168, 222-222, 252-252, 334-334, 402-402, 452-452, 533-533, 599-599
115-171:✅ Verification successful
Add error handling as requested in PR comments
The new query looks good, but remember to add try-catch blocks to any functions that will use this query, as requested in the PR comments by sudeepb02.
When implementing functions that use this query, wrap the execution in try-catch blocks that return 0 on error:
🏁 Script executed:
#!/bin/bash # Check if functions using this query have proper error handling rg -A 3 -B 3 "fetchOpenPositionsByUser" src/Length of output: 1518
🏁 Script executed:
#!/bin/bash # Show catch block implementation for fetchOpenPositionsByUser in src/subgraph/positions/index.ts rg -n -C 5 "catch" src/subgraph/positions/index.tsLength of output: 4265
Error handling already in place for fetchOpenPositionsByUser
The
fetchOpenPositionsByUserwrapper insrc/subgraph/positions/index.ts(lines 46–55) already includes a try–catch that logs errors and returns an empty array on failure, consistent with the other position‐fetching functions. No further changes are needed.src/interfaces/sdkTypes.ts (1)
1-217:❌ Incorrect review comment
Add the requested try-catch blocks to all function implementations
While the interface changes look good, remember that PR feedback requested adding try-catch blocks to functions to handle errors gracefully and return 0 on errors. This should be implemented in the actual function implementations, not just in the type definitions.
Let's check if this was implemented in any of the actual function implementation files:
🏁 Script executed:
#!/bin/bash # Check if the distribution code functions have try-catch blocks # Check src/distribution directory if it exists if [ -d "src/distribution" ]; then echo "Distribution directory exists, checking for try-catch blocks:" rg -A 2 "try\s*{" src/distribution/ echo "-----------------" echo "Checking for functions without try-catch:" rg -l "function|=>\s*{" src/distribution/ | xargs -I{} bash -c "if ! rg -q 'try\s*{' {}; then echo '{}'; fi" else echo "Distribution directory not found - this should exist based on PR description" fiLength of output: 633
I’ll run a more robust check for
try { … } catchin all distribution functions:#!/usr/bin/env bash set -euo pipefail if [ -d "src/distribution" ]; then echo "Scanning src/distribution for functions missing try-catch blocks:" # Find all .ts/.js files under src/distribution mapfile -t files < <(rg -l -e "function" -e "=>" src/distribution/) for f in "${files[@]}"; do if grep -E "try[[:space:]]*\{" "$f" >/dev/null; then echo " ✓ $f" else echo " ✗ $f" fi done else echo "ERROR: src/distribution directory not found" fi
I’ll verify whether any
try { … } catchblocks exist insrc/distributionand list all function definitions there:#!/usr/bin/env bash set -euo pipefail if [ -d "src/distribution" ]; then echo "🔍 Files containing a `try {` in src/distribution:" rg -l 'try {' src/distribution/ || echo " (none found)" echo echo "🔍 All function declarations/arrow functions in src/distribution:" rg -l -E '^\s*function\s+\w+|\w+\s*=\s*\(.*\)\s*=>' src/distribution/ || echo " (none found)" else echo "❌ ERROR: src/distribution directory not found" fi
I’ll run a more robust check for
try { … } catchin all distribution functions:#!/usr/bin/env bash set -euo pipefail if [ ! -d "src/distribution" ]; then echo "❌ src/distribution directory not found" exit 0 fi echo "🔍 Files in src/distribution containing a try-catch:" rg -n -P 'try\s*\{' src/distribution/ || echo " (none found)" echo echo "🔍 All function declarations and arrow functions in src/distribution:" rg -n -e '^\s*function\s+\w+' -e '^\s*\w+\s*=\s*\(.*\)\s*=>' src/distribution/ || echo " (none found)"
I’ll run a repo-wide check for
try { … } catchin all JS/TS functions to see which files still need error handling:#!/usr/bin/env bash set -euo pipefail echo "🔍 Files containing a try block:" rg -lP 'try\s*\{' -t ts -t js src/ || echo " (none found)" echo echo "🔍 Functions declared in .ts/.js and missing any try block:" # Find all .ts/.js files with function or arrow fn, then flag those without try rg -lE '^\s*(function\s+\w+|\w+\s*=\s*\([^)]*\)\s*=>)' -t ts -t js src/ | while read -r file; do if ! grep -qP 'try\s*\{' "$file"; then echo " ✗ $file" fi done
Remove try-catch request for type definitions
src/interfaces/sdkTypes.tsonly defines TypeScript types/interfaces and contains no executable functions to wrap intry…catch. The requested error‐handling should be implemented in the actual function bodies (for example, undersrc/distributionor wherever those functions are defined), not in this file.Likely an incorrect or invalid review comment.
test/subgraph-tests/orders.test.ts
Outdated
| it.only('should return an settled order count for a user', async () => { | ||
| const parifiSdk = await getParifiSdkInstanceForTesting(); | ||
| const address = '0x680f7cF1C802F5C1C5A93e3657AAA3a1152ccd44'; | ||
| const startTime = 1744771331; | ||
| const endTime = 1745223131; | ||
| const orderCount = await parifiSdk.subgraph.getSettledOrdersCountBasedOnTimeStampByUserAddress( | ||
| address, | ||
| 50, | ||
| 0, | ||
| startTime, | ||
| endTime, | ||
| ); | ||
| console.log(orderCount); | ||
| expect(orderCount).toBe(2); | ||
| }); |
There was a problem hiding this comment.
Remove .only from test and add error handling
The .only modifier causes only this test to run, skipping all others. This should be removed before merging. Additionally, consider adding a try-catch block to handle potential errors as requested in the PR comments.
- it.only('should return an settled order count for a user', async () => {
+ it('should return an settled order count for a user', async () => {
const parifiSdk = await getParifiSdkInstanceForTesting();
const address = '0x680f7cF1C802F5C1C5A93e3657AAA3a1152ccd44';
const startTime = 1744771331;
const endTime = 1745223131;
- const orderCount = await parifiSdk.subgraph.getSettledOrdersCountBasedOnTimeStampByUserAddress(
- address,
- 50,
- 0,
- startTime,
- endTime,
- );
- console.log(orderCount);
- expect(orderCount).toBe(2);
+ try {
+ const orderCount = await parifiSdk.subgraph.getSettledOrdersCountBasedOnTimeStampByUserAddress(
+ address,
+ 50,
+ 0,
+ startTime,
+ endTime,
+ );
+ expect(orderCount).toBe(2);
+ } catch (error) {
+ console.error('Error fetching settled orders count:', error);
+ expect(0).toBe(2); // This will fail the test if an error occurs
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it.only('should return an settled order count for a user', async () => { | |
| const parifiSdk = await getParifiSdkInstanceForTesting(); | |
| const address = '0x680f7cF1C802F5C1C5A93e3657AAA3a1152ccd44'; | |
| const startTime = 1744771331; | |
| const endTime = 1745223131; | |
| const orderCount = await parifiSdk.subgraph.getSettledOrdersCountBasedOnTimeStampByUserAddress( | |
| address, | |
| 50, | |
| 0, | |
| startTime, | |
| endTime, | |
| ); | |
| console.log(orderCount); | |
| expect(orderCount).toBe(2); | |
| }); | |
| it('should return an settled order count for a user', async () => { | |
| const parifiSdk = await getParifiSdkInstanceForTesting(); | |
| const address = '0x680f7cF1C802F5C1C5A93e3657AAA3a1152ccd44'; | |
| const startTime = 1744771331; | |
| const endTime = 1745223131; | |
| try { | |
| const orderCount = await parifiSdk.subgraph.getSettledOrdersCountBasedOnTimeStampByUserAddress( | |
| address, | |
| 50, | |
| 0, | |
| startTime, | |
| endTime, | |
| ); | |
| expect(orderCount).toBe(2); | |
| } catch (error) { | |
| console.error('Error fetching settled orders count:', error); | |
| expect(0).toBe(2); // This will fail the test if an error occurs | |
| } | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
| // Fetch all orders by `userAddress` | ||
| export const fetchIntegratorFeesWithTimestampQuery = ( | ||
| startTimeStamp: number, // unix timestamp | ||
| endTimeStamp: number, //unix timestamp | ||
| ) => | ||
| gql`{ | ||
| snxAccounts( | ||
| first: 1000 | ||
| where: { type: PERP, totalOrdersCount_gt: 0 }) { | ||
| id | ||
| integratorFeesGenerated | ||
| accountId | ||
| owner { | ||
| id | ||
| } | ||
| orders( | ||
| where: { | ||
| status: SETTLED, | ||
| createdTimestamp_gt: ${startTimeStamp}, | ||
| createdTimestamp_lt: ${endTimeStamp} | ||
| } | ||
| ) { | ||
| id | ||
| referralFees | ||
| collectedFees | ||
| } | ||
| } | ||
| } | ||
| `; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add try-catch block to handle potential errors
As requested in the PR comments, all new functions should include error handling. Consider implementing a try-catch block in the function that uses this query to handle potential errors gracefully and return 0 in case of failures.
export const fetchIntegratorFeesWithTimestampQuery = (
startTimeStamp: number, // unix timestamp
endTimeStamp: number, //unix timestamp
) =>
gql`{
snxAccounts(
first: 1000
where: { type: PERP, totalOrdersCount_gt: 0 }) {
id
integratorFeesGenerated
accountId
owner {
id
}
orders(
where: {
status: SETTLED,
createdTimestamp_gt: ${startTimeStamp},
createdTimestamp_lt: ${endTimeStamp}
}
) {
id
referralFees
collectedFees
}
}
}
`;Additionally, functions using this query should implement error handling like:
export const getIntegratorFeesWithinTimeRange = async (
subgraphEndpoint: string,
startTimeStamp: number,
endTimeStamp: number
) => {
try {
const query = fetchIntegratorFeesWithTimestampQuery(startTimeStamp, endTimeStamp);
const response = await request(subgraphEndpoint, query);
// Process response data
return response;
} catch (error) {
console.error('Error fetching integrator fees:', error);
return 0; // Return 0 in case of error as requested
}
};
src/subgraph/scripts/index.ts
Outdated
| export const getEstimatedRewardsForUser = async ( | ||
| subgraphEndpoint: string, | ||
| userAddress: string, | ||
| totalRewardsForPeriod: number, | ||
| startTimestamp: number, | ||
| endTimestamp: number, | ||
| ): Promise<number> => { | ||
| interface FeeDetailsInterface { | ||
| id: string; | ||
| integratorFeesGenerated: string; | ||
| accountId: string; | ||
| owner: { id: string }; | ||
| orders: { | ||
| id: string; | ||
| referralFees: string; | ||
| collectedFees: string; | ||
| }[]; | ||
| } | ||
|
|
||
| const response: any = await request( | ||
| subgraphEndpoint, | ||
| fetchIntegratorFeesWithTimestampQuery(startTimestamp, endTimestamp), | ||
| ); | ||
| const feeDetails: FeeDetailsInterface[] = response?.snxAccounts ?? []; | ||
|
|
||
| let totalFeesForPeriod = 0; | ||
| let userFeesForPeriod = 0; | ||
| feeDetails.forEach((account) => { | ||
| account.orders.forEach((order) => { | ||
| const formattedFees = new Decimal(order.referralFees).div(WAD); | ||
| if (account.owner.id == userAddress) { | ||
| userFeesForPeriod += Number(formattedFees.toString()); | ||
| } | ||
| totalFeesForPeriod += Number(formattedFees.toString()); | ||
| }); | ||
| }); | ||
|
|
||
| if (totalFeesForPeriod === 0) { | ||
| return 0; | ||
| } | ||
| const estimatedUserRewards = (userFeesForPeriod * totalRewardsForPeriod) / totalFeesForPeriod; | ||
| return estimatedUserRewards; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add try-catch block and fix equality comparison
As requested in the PR comments, please add a try-catch block to handle potential errors gracefully. Also, the comparison on line 36 should use strict equality (===) instead of loose equality (==).
export const getEstimatedRewardsForUser = async (
subgraphEndpoint: string,
userAddress: string,
totalRewardsForPeriod: number,
startTimestamp: number,
endTimestamp: number,
): Promise<number> => {
interface FeeDetailsInterface {
id: string;
integratorFeesGenerated: string;
accountId: string;
owner: { id: string };
orders: {
id: string;
referralFees: string;
collectedFees: string;
}[];
}
+ try {
const response: any = await request(
subgraphEndpoint,
fetchIntegratorFeesWithTimestampQuery(startTimestamp, endTimestamp),
);
const feeDetails: FeeDetailsInterface[] = response?.snxAccounts ?? [];
let totalFeesForPeriod = 0;
let userFeesForPeriod = 0;
feeDetails.forEach((account) => {
account.orders.forEach((order) => {
const formattedFees = new Decimal(order.referralFees).div(WAD);
- if (account.owner.id == userAddress) {
+ if (account.owner.id === userAddress) {
userFeesForPeriod += Number(formattedFees.toString());
}
totalFeesForPeriod += Number(formattedFees.toString());
});
});
if (totalFeesForPeriod === 0) {
return 0;
}
const estimatedUserRewards = (userFeesForPeriod * totalRewardsForPeriod) / totalFeesForPeriod;
return estimatedUserRewards;
+ } catch (error) {
+ console.error('Error estimating rewards for user:', error);
+ return 0;
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getEstimatedRewardsForUser = async ( | |
| subgraphEndpoint: string, | |
| userAddress: string, | |
| totalRewardsForPeriod: number, | |
| startTimestamp: number, | |
| endTimestamp: number, | |
| ): Promise<number> => { | |
| interface FeeDetailsInterface { | |
| id: string; | |
| integratorFeesGenerated: string; | |
| accountId: string; | |
| owner: { id: string }; | |
| orders: { | |
| id: string; | |
| referralFees: string; | |
| collectedFees: string; | |
| }[]; | |
| } | |
| const response: any = await request( | |
| subgraphEndpoint, | |
| fetchIntegratorFeesWithTimestampQuery(startTimestamp, endTimestamp), | |
| ); | |
| const feeDetails: FeeDetailsInterface[] = response?.snxAccounts ?? []; | |
| let totalFeesForPeriod = 0; | |
| let userFeesForPeriod = 0; | |
| feeDetails.forEach((account) => { | |
| account.orders.forEach((order) => { | |
| const formattedFees = new Decimal(order.referralFees).div(WAD); | |
| if (account.owner.id == userAddress) { | |
| userFeesForPeriod += Number(formattedFees.toString()); | |
| } | |
| totalFeesForPeriod += Number(formattedFees.toString()); | |
| }); | |
| }); | |
| if (totalFeesForPeriod === 0) { | |
| return 0; | |
| } | |
| const estimatedUserRewards = (userFeesForPeriod * totalRewardsForPeriod) / totalFeesForPeriod; | |
| return estimatedUserRewards; | |
| }; | |
| export const getEstimatedRewardsForUser = async ( | |
| subgraphEndpoint: string, | |
| userAddress: string, | |
| totalRewardsForPeriod: number, | |
| startTimestamp: number, | |
| endTimestamp: number, | |
| ): Promise<number> => { | |
| interface FeeDetailsInterface { | |
| id: string; | |
| integratorFeesGenerated: string; | |
| accountId: string; | |
| owner: { id: string }; | |
| orders: { | |
| id: string; | |
| referralFees: string; | |
| collectedFees: string; | |
| }[]; | |
| } | |
| try { | |
| const response: any = await request( | |
| subgraphEndpoint, | |
| fetchIntegratorFeesWithTimestampQuery(startTimestamp, endTimestamp), | |
| ); | |
| const feeDetails: FeeDetailsInterface[] = response?.snxAccounts ?? []; | |
| let totalFeesForPeriod = 0; | |
| let userFeesForPeriod = 0; | |
| feeDetails.forEach((account) => { | |
| account.orders.forEach((order) => { | |
| const formattedFees = new Decimal(order.referralFees).div(WAD); | |
| if (account.owner.id === userAddress) { | |
| userFeesForPeriod += Number(formattedFees.toString()); | |
| } | |
| totalFeesForPeriod += Number(formattedFees.toString()); | |
| }); | |
| }); | |
| if (totalFeesForPeriod === 0) { | |
| return 0; | |
| } | |
| const estimatedUserRewards = | |
| (userFeesForPeriod * totalRewardsForPeriod) / totalFeesForPeriod; | |
| return estimatedUserRewards; | |
| } catch (error) { | |
| console.error('Error estimating rewards for user:', error); | |
| return 0; | |
| } | |
| }; |
src/common/subgraphMapper.ts
Outdated
| realizedPositionPnl: response?.realizedPositionPnl, | ||
| totalFeesPaid: response?.totalFeesPaid, | ||
| realizedPnlAfterFees: response?.realizedPnlAfterFees, | ||
| createdTimestamp: response?.createdTimestamp, | ||
| lastRefresh: response?.lastRefresh, | ||
| lastRefreshISO: response?.lastRefreshISO, | ||
| accruedBorrowingFees: response?.accruedBorrowingFees, | ||
| canBeLiquidated: response?.canBeLiquidated, | ||
| snapshotCollateralValueUsd: response?.snapshotCollateralValueUsd, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add comments for position financial metrics
The renamed fields in the position mapping need clear explanatory comments. Also, consider implementing the requested error handling that returns 0 values on error.
- realizedPositionPnl: response?.realizedPositionPnl,
- totalFeesPaid: response?.totalFeesPaid,
- realizedPnlAfterFees: response?.realizedPnlAfterFees,
+ // Raw P&L from the position before fees
+ realizedPositionPnl: response?.realizedPositionPnl,
+ // Total fees paid for this position
+ totalFeesPaid: response?.totalFeesPaid,
+ // Final P&L after accounting for all fees
+ realizedPnlAfterFees: response?.realizedPnlAfterFees,
createdTimestamp: response?.createdTimestamp,
lastRefresh: response?.lastRefresh,
lastRefreshISO: response?.lastRefreshISO,
accruedBorrowingFees: response?.accruedBorrowingFees,
canBeLiquidated: response?.canBeLiquidated,
+ // USD value of the collateral at position snapshot time
snapshotCollateralValueUsd: response?.snapshotCollateralValueUsd,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| realizedPositionPnl: response?.realizedPositionPnl, | |
| totalFeesPaid: response?.totalFeesPaid, | |
| realizedPnlAfterFees: response?.realizedPnlAfterFees, | |
| createdTimestamp: response?.createdTimestamp, | |
| lastRefresh: response?.lastRefresh, | |
| lastRefreshISO: response?.lastRefreshISO, | |
| accruedBorrowingFees: response?.accruedBorrowingFees, | |
| canBeLiquidated: response?.canBeLiquidated, | |
| snapshotCollateralValueUsd: response?.snapshotCollateralValueUsd, | |
| // Raw P&L from the position before fees | |
| realizedPositionPnl: response?.realizedPositionPnl, | |
| // Total fees paid for this position | |
| totalFeesPaid: response?.totalFeesPaid, | |
| // Final P&L after accounting for all fees | |
| realizedPnlAfterFees: response?.realizedPnlAfterFees, | |
| createdTimestamp: response?.createdTimestamp, | |
| lastRefresh: response?.lastRefresh, | |
| lastRefreshISO: response?.lastRefreshISO, | |
| accruedBorrowingFees: response?.accruedBorrowingFees, | |
| canBeLiquidated: response?.canBeLiquidated, | |
| // USD value of the collateral at position snapshot time | |
| snapshotCollateralValueUsd: response?.snapshotCollateralValueUsd, |
src/common/subgraphMapper.ts
Outdated
| cancellationTxHash: response?.cancellationTxHash, | ||
| settledTimestamp: response?.settledTimestamp, | ||
| settledTimestampISO: response?.settledTimestampISO, | ||
| settledBy: response?.settledBy ?? mapResponseToWallet(response?.settledBy), | ||
| snapshotCollateralValueUsd: response?.snapshotCollateralValueUsd, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for order mapping
Per the PR feedback, we should implement try-catch blocks that return 0 on error. The current error handling just logs and returns undefined.
export const mapResponseToOrder = (response: any): Order | undefined => {
if (!response) return undefined;
try {
return {
id: response?.id,
market: response?.market ?? mapResponseToMarket(response?.market),
snxAccountId: response?.snxAccount?.id,
isLimitOrder: response?.isLimitOrder,
acceptablePrice: response?.acceptablePrice,
commitmentTime: response?.commitmentTime,
expectedPriceTime: response?.expectedPriceTime,
settlementTime: response?.settlementTime,
expirationTime: response?.expirationTime,
trackingCode: response?.trackingCode,
deltaSize: response?.deltaSize,
deltaSizeUsd: response?.deltaSizeUsd,
executionPrice: response?.executionPrice,
collectedFees: response?.collectedFees,
settlementReward: response?.settlementReward,
referralFees: response?.referralFees,
partnerAddress: response?.partnerAddress,
txHash: response?.txHash,
createdTimestamp: response?.createdTimestamp,
status: response?.status,
settledTxHash: response?.settledTxHash,
cancellationTxHash: response?.cancellationTxHash,
settledTimestamp: response?.settledTimestamp,
settledTimestampISO: response?.settledTimestampISO,
settledBy: response?.settledBy ?? mapResponseToWallet(response?.settledBy),
snapshotCollateralValueUsd: response?.snapshotCollateralValueUsd,
};
} catch (error) {
console.log('Error while mapping data', error);
- return undefined;
+ // Return a default order with zeroed values as requested in PR feedback
+ return {
+ id: response?.id || '',
+ deltaSize: '0',
+ deltaSizeUsd: '0',
+ executionPrice: '0',
+ collectedFees: '0',
+ settlementReward: '0',
+ referralFees: '0',
+ snapshotCollateralValueUsd: '0'
+ };
}
};Committable suggestion skipped: line range outside the PR's diff.
src/interfaces/sdkTypes.ts
Outdated
| export type SettlementStrategy = { | ||
| id?: string; | ||
| strategyId?: string; | ||
| strategyType?: string; | ||
| settlementDelay?: string; | ||
| settlementWindowDuration?: string; | ||
| priceVerificationContract?: string; | ||
| feedId?: string; | ||
| settlementReward?: string; | ||
| disabled?: string; | ||
| commitmentPriceDelay?: string; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc comment for SettlementStrategy type
The new SettlementStrategy type should have a JSDoc comment explaining its purpose and relationship to the settlement process.
+/**
+ * Represents a settlement strategy configuration for orders.
+ * Controls parameters like settlement delay, window duration,
+ * and rewards for settlement.
+ */
export type SettlementStrategy = {
id?: string;
strategyId?: string;
strategyType?: string;
settlementDelay?: string;
settlementWindowDuration?: string;
priceVerificationContract?: string;
feedId?: string;
settlementReward?: string;
disabled?: string;
commitmentPriceDelay?: string;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type SettlementStrategy = { | |
| id?: string; | |
| strategyId?: string; | |
| strategyType?: string; | |
| settlementDelay?: string; | |
| settlementWindowDuration?: string; | |
| priceVerificationContract?: string; | |
| feedId?: string; | |
| settlementReward?: string; | |
| disabled?: string; | |
| commitmentPriceDelay?: string; | |
| }; | |
| /** | |
| * Represents a settlement strategy configuration for orders. | |
| * Controls parameters like settlement delay, window duration, | |
| * and rewards for settlement. | |
| */ | |
| export type SettlementStrategy = { | |
| id?: string; | |
| strategyId?: string; | |
| strategyType?: string; | |
| settlementDelay?: string; | |
| settlementWindowDuration?: string; | |
| priceVerificationContract?: string; | |
| feedId?: string; | |
| settlementReward?: string; | |
| disabled?: string; | |
| commitmentPriceDelay?: string; | |
| }; |
src/interfaces/sdkTypes.ts
Outdated
| export interface ProtocolStats { | ||
| id?: string; | ||
| userCount?: string; | ||
| activeUsersCount?: string; | ||
| orderCount?: string; | ||
| settledCount?: string; | ||
| positionCount?: string; | ||
| totalActivePositions?: string; | ||
| referralFeesReceived?: string; | ||
| totalVolume?: string; | ||
| orderTotalFees?: string; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc comment for ProtocolStats interface
The new ProtocolStats interface should have a JSDoc comment describing its purpose and usage within the SDK.
+/**
+ * Contains global protocol statistics such as user counts,
+ * order counts, and volume metrics
+ */
export interface ProtocolStats {
id?: string;
userCount?: string;
activeUsersCount?: string;
orderCount?: string;
settledCount?: string;
positionCount?: string;
totalActivePositions?: string;
referralFeesReceived?: string;
totalVolume?: string;
orderTotalFees?: string;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface ProtocolStats { | |
| id?: string; | |
| userCount?: string; | |
| activeUsersCount?: string; | |
| orderCount?: string; | |
| settledCount?: string; | |
| positionCount?: string; | |
| totalActivePositions?: string; | |
| referralFeesReceived?: string; | |
| totalVolume?: string; | |
| orderTotalFees?: string; | |
| } | |
| /** | |
| * Contains global protocol statistics such as user counts, | |
| * order counts, and volume metrics | |
| */ | |
| export interface ProtocolStats { | |
| id?: string; | |
| userCount?: string; | |
| activeUsersCount?: string; | |
| orderCount?: string; | |
| settledCount?: string; | |
| positionCount?: string; | |
| totalActivePositions?: string; | |
| referralFeesReceived?: string; | |
| totalVolume?: string; | |
| orderTotalFees?: string; | |
| } |
src/interfaces/sdkTypes.ts
Outdated
| realizedPnlFromPositions?: string; | ||
| totalFeesPaid?: string; | ||
| finalPnlAfterFees?: string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc comments to explain the fields
These renamed/new fields should have JSDoc comments explaining their purpose and usage, as requested in the PR feedback.
+ /** Total realized profit/loss from all positions */
realizedPnlFromPositions?: string;
+ /** Total fees paid across all positions and orders */
totalFeesPaid?: string;
+ /** Final P&L after accounting for all fees */
finalPnlAfterFees?: string;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| realizedPnlFromPositions?: string; | |
| totalFeesPaid?: string; | |
| finalPnlAfterFees?: string; | |
| /** Total realized profit/loss from all positions */ | |
| realizedPnlFromPositions?: string; | |
| /** Total fees paid across all positions and orders */ | |
| totalFeesPaid?: string; | |
| /** Final P&L after accounting for all fees */ | |
| finalPnlAfterFees?: string; |
src/interfaces/sdkTypes.ts
Outdated
| realizedPositionPnl?: string; | ||
| totalFeesPaid?: string; | ||
| realizedPnlAfterFees?: string; | ||
| createdTimestamp?: string; | ||
| lastRefresh?: string; | ||
| lastRefreshISO?: string; | ||
| accruedBorrowingFees?: string; | ||
| canBeLiquidated?: boolean; | ||
| snapshotCollateralValueUsd?: string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc comments for Position financial metrics
The renamed and new fields in the Position type should have JSDoc comments to improve clarity.
+ /** Raw P&L from the position before fees */
realizedPositionPnl?: string;
+ /** Total fees paid for this position */
totalFeesPaid?: string;
+ /** Final P&L after accounting for all fees */
realizedPnlAfterFees?: string;
createdTimestamp?: string;
lastRefresh?: string;
lastRefreshISO?: string;
accruedBorrowingFees?: string;
canBeLiquidated?: boolean;
+ /** USD value of the account collateral at position snapshot time */
snapshotCollateralValueUsd?: string;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| realizedPositionPnl?: string; | |
| totalFeesPaid?: string; | |
| realizedPnlAfterFees?: string; | |
| createdTimestamp?: string; | |
| lastRefresh?: string; | |
| lastRefreshISO?: string; | |
| accruedBorrowingFees?: string; | |
| canBeLiquidated?: boolean; | |
| snapshotCollateralValueUsd?: string; | |
| /** Raw P&L from the position before fees */ | |
| realizedPositionPnl?: string; | |
| /** Total fees paid for this position */ | |
| totalFeesPaid?: string; | |
| /** Final P&L after accounting for all fees */ | |
| realizedPnlAfterFees?: string; | |
| createdTimestamp?: string; | |
| lastRefresh?: string; | |
| lastRefreshISO?: string; | |
| accruedBorrowingFees?: string; | |
| canBeLiquidated?: boolean; | |
| /** USD value of the account collateral at position snapshot time */ | |
| snapshotCollateralValueUsd?: string; |
src/interfaces/sdkTypes.ts
Outdated
| maxMarketSize?: string; | ||
| marketPrice?: string; | ||
| initialMarginRatioD18?: string; | ||
| maintenanceMarginRatioD18?: string; | ||
| minimumInitialMarginRatioD18?: string; | ||
| flagRewardRatioD18?: string; | ||
| minimumPositionMargin?: string; | ||
| openInterestUsd?: string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add JSDoc comments for Market properties
The newly added fields in the Market type need explanatory JSDoc comments, especially for technical parameters like flagRewardRatioD18.
+ /** Maximum market size in base units */
maxMarketSize?: string;
marketPrice?: string;
initialMarginRatioD18?: string;
maintenanceMarginRatioD18?: string;
minimumInitialMarginRatioD18?: string;
flagRewardRatioD18?: string;
minimumPositionMargin?: string;
+ /** Total open interest denominated in USD */
openInterestUsd?: string;
racso2609
force-pushed
the
feature/merkle-distribution
branch
from
c698559 to
7504f6e
Compare
Summary by CodeRabbit
New Features
Chores