feat: pwn subgraph #41
Conversation
|
/korbit-review |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Inefficient Byte Conversion Chain ▹ view | |
 |
Missing Error Handling ▹ view | |
 |
Unclear underscore prefix in variable name ▹ view | |
|
Complex One-liner for Byte Conversion ▹ view | |
|
Missing Account Cache for Repeated Lookups ▹ view | |
 |
Redundant Pass-Through Function ▹ view | |
|
Missing Critical Logging Implementation ▹ view | |
|
Inefficient Entity Creation Pattern ▹ view | |
|
Remove Dead Code ▹ view | |
 |
Unsafe Feed ID Generation ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| packages/subgraph/src/hub.ts | ✅ |
| packages/subgraph/src/helpers.ts | ✅ |
| packages/subgraph/src/multi-token-category-registry.ts | ✅ |
| packages/subgraph/src/chainlink-feed-registry.ts | ✅ |
| packages/v1-core/src/contracts/elastic-proposal-contract.ts | ✅ |
| packages/v1-core/src/contracts/chain-link-proposal-contract.ts | ✅ |
| packages/subgraph/src/loan-token.ts | ✅ |
| packages/subgraph/src/simple-loan-simple-proposal.ts | ✅ |
| packages/subgraph/src/simple-loan-elastic-proposal.ts | ✅ |
| packages/subgraph/src/simple-loan-list-proposal.ts | ✅ |
| packages/subgraph/src/simple-loan-uniswap-v-3-lp-set-proposal.ts | ✅ |
| packages/subgraph/src/simple-loan-uniswap-v-3-lp-individual-proposal.ts | ✅ |
| packages/subgraph/src/config.ts | ✅ |
| packages/subgraph/src/simple-loan-elastic-chainlink-proposal.ts | ✅ |
| packages/subgraph/src/utils.ts | ✅ |
| packages/subgraph/src/simple-loan.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| entity.blockTimestamp = event.block.timestamp | ||
| entity.transactionHash = event.transaction.hash | ||
|
|
||
| entity.save() |
There was a problem hiding this comment.
Missing Error Handling 
Tell me more
What is the issue?
No error handling is implemented for the entity save operation, which could fail silently.
Why this matters
If the save operation fails, the event data would be lost without any indication of the failure, potentially leading to incomplete or inconsistent data in the subgraph.
Suggested change ∙ Feature Preview
Implement try-catch block for error handling:
try {
entity.save()
} catch (error) {
log.error('Error saving TagSetEvent entity: {}', [error.toString()])
throw error
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| const entity = new TagSetEventEntity( | ||
| event.transaction.hash.concatI32(event.logIndex.toI32()), | ||
| ) | ||
| entity._address = event.params._address |
There was a problem hiding this comment.
Unclear underscore prefix in variable name 
Tell me more
What is the issue?
Variable name starts with underscore without clear indication of why the prefix is used.
Why this matters
In TypeScript, leading underscores typically indicate private members or unused variables. Using it here without clear intent can confuse readers about the variable's purpose or scope.
Suggested change ∙ Feature Preview
entity.address = event.params.address // Remove underscore if not needed for privacy/unused variable indicationProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| export function getAssetId(assetAddress: Address, assetId: BigInt): Bytes { | ||
| return assetAddress.concat(Bytes.fromByteArray(Bytes.fromBigInt(assetId))); | ||
| } |
There was a problem hiding this comment.
Complex One-liner for Byte Conversion
Tell me more
What is the issue?
The nested function calls in a single line make the byte conversion and concatenation operations hard to follow.
Why this matters
Complex one-liners reduce code readability and make debugging more difficult. Breaking down the operations into steps with intermediate variables makes the code's intent clearer.
Suggested change ∙ Feature Preview
export function getAssetId(assetAddress: Address, assetId: BigInt): Bytes {
const assetIdBytes = Bytes.fromBigInt(assetId);
const assetIdByteArray = Bytes.fromByteArray(assetIdBytes);
return assetAddress.concat(assetIdByteArray);
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| export function getOrCreateAccount(address: Address): Account { | ||
| let account = Account.load(address); | ||
| if (account == null) { | ||
| account = new Account(address); | ||
| account.save(); | ||
| } | ||
| return account; | ||
| } |
There was a problem hiding this comment.
Missing Account Cache for Repeated Lookups 
Tell me more
What is the issue?
Each 'getOrCreateAccount' call performs a database load operation and potentially a save operation without any caching mechanism.
Why this matters
In high-frequency scenarios where the same account is accessed multiple times, repeated database operations can create unnecessary I/O overhead and impact performance.
Suggested change ∙ Feature Preview
Implement an in-memory cache for recently accessed accounts using a Map data structure. Example:
let accountCache = new Map<string, Account>();
export function getOrCreateAccount(address: Address): Account {
let id = address.toHexString();
let cached = accountCache.get(id);
if (cached) return cached;
let account = Account.load(address);
if (account == null) {
account = new Account(address);
account.save();
}
accountCache.set(id, account);
return account;
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| } | ||
|
|
||
| export function getAssetId(assetAddress: Address, assetId: BigInt): Bytes { | ||
| return assetAddress.concat(Bytes.fromByteArray(Bytes.fromBigInt(assetId))); |
There was a problem hiding this comment.
Inefficient Byte Conversion Chain
Tell me more
What is the issue?
Multiple unnecessary byte conversions in getAssetId function creating temporary byte arrays.
Why this matters
Each conversion (BigInt to Bytes to ByteArray back to Bytes) creates temporary objects and performs redundant operations, impacting performance with unnecessary memory allocations.
Suggested change ∙ Feature Preview
Simplify the byte conversion process by using a direct conversion method:
export function getAssetId(assetAddress: Address, assetId: BigInt): Bytes {
return assetAddress.concat(Bytes.fromBigInt(assetId));
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| export function getAssetContractId(assetAddress: Address): Bytes { | ||
| return assetAddress; | ||
| } |
There was a problem hiding this comment.
Redundant Pass-Through Function 
Tell me more
What is the issue?
The getAssetContractId function serves as a simple pass-through without any transformation or business logic, making it an unnecessary abstraction.
Why this matters
Unnecessary abstractions increase code complexity and maintenance overhead without providing any benefits. In this case, callers could directly use the assetAddress.
Suggested change ∙ Feature Preview
Either remove the function and use assetAddress directly at call sites, or add meaningful transformation logic if required:
// If additional logic is needed:
export function getAssetContractId(assetAddress: Address): Bytes {
// Add necessary transformation or validation
return assetAddress;
}
// Otherwise, remove the function entirelyProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| // TODO add logs where applicable | ||
| export function getOrCreateAccount(address: Address): Account { | ||
| let account = Account.load(address); | ||
| if (account == null) { | ||
| account = new Account(address); | ||
| account.save(); | ||
| } | ||
| return account; | ||
| } |
There was a problem hiding this comment.
Missing Critical Logging Implementation
Tell me more
What is the issue?
The TODO comment indicates missing logging functionality, which is important for monitoring and debugging in a production environment.
Why this matters
Without proper logging, it becomes difficult to track account creation and loading operations, hampering debugging and monitoring capabilities.
Suggested change ∙ Feature Preview
Implement appropriate logging:
export function getOrCreateAccount(address: Address): Account {
let account = Account.load(address);
if (account == null) {
log.info('Creating new account for address {}', [address.toHexString()]);
account = new Account(address);
account.save();
}
return account;
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| let chainlinkFeed = ChainlinkFeed.load(chainlinkFeedId) | ||
| if (chainlinkFeed == null) { | ||
| chainlinkFeed = new ChainlinkFeed(chainlinkFeedId) | ||
| } |
There was a problem hiding this comment.
Inefficient Entity Creation Pattern
Tell me more
What is the issue?
Redundant entity load operation when creating new entities in high-frequency event handlers.
Why this matters
Loading entities from storage is an expensive operation. For high-frequency events, this check-then-create pattern adds unnecessary overhead when most calls will create new entities.
Suggested change ∙ Feature Preview
If the business logic allows, consider using a more performant pattern that assumes new creation:
let chainlinkFeed = new ChainlinkFeed(chainlinkFeedId)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| // const entity = new FeedConfirmed( | ||
| // event.transaction.hash.concatI32(event.logIndex.toI32()), | ||
| // ) | ||
| // entity.asset = event.params.asset | ||
| // entity.denomination = event.params.denomination | ||
| // entity.latestAggregator = event.params.latestAggregator | ||
| // entity.previousAggregator = event.params.previousAggregator | ||
| // entity.nextPhaseId = event.params.nextPhaseId | ||
| // entity.sender = event.params.sender | ||
|
|
||
| // entity.blockNumber = event.block.number | ||
| // entity.blockTimestamp = event.block.timestamp | ||
| // entity.transactionHash = event.transaction.hash | ||
|
|
||
| // entity.save() | ||
| } | ||
|
|
||
| // export function handleFeedProposed(event: FeedProposedEvent): void { | ||
| // const entity = new FeedProposed( | ||
| // event.transaction.hash.concatI32(event.logIndex.toI32()), | ||
| // ) | ||
| // entity.asset = event.params.asset | ||
| // entity.denomination = event.params.denomination | ||
| // entity.proposedAggregator = event.params.proposedAggregator | ||
| // entity.currentAggregator = event.params.currentAggregator | ||
| // entity.sender = event.params.sender | ||
|
|
||
| // entity.blockNumber = event.block.number | ||
| // entity.blockTimestamp = event.block.timestamp | ||
| // entity.transactionHash = event.transaction.hash | ||
|
|
||
| // entity.save() | ||
| // } |
There was a problem hiding this comment.
Remove Dead Code
Tell me more
What is the issue?
Large blocks of commented-out code are left in the codebase, creating unnecessary cognitive load and maintenance burden.
Why this matters
Commented code creates confusion about whether it might be needed in the future, makes the codebase harder to maintain, and clutters the source file. Version control systems should be used to track code history instead.
Suggested change ∙ Feature Preview
Either remove the commented code entirely or, if it represents important logic that might be needed later, document it properly in documentation or tickets rather than leaving it as comments in the source code.
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| function getChainlinkFeedId(event: FeedConfirmedEvent): Bytes { | ||
| return event.params.asset.concat(event.params.denomination) | ||
| } |
There was a problem hiding this comment.
Unsafe Feed ID Generation 
Tell me more
What is the issue?
The chainlink feed ID construction using simple concatenation of asset and denomination could lead to ID collisions.
Why this matters
If two different pairs of assets and denominations happen to have the same concatenated value (e.g., 'ETHUSDT' could be either ETH/USDT or ET/HUSDT), it would cause incorrect data overwrites in the database.
Suggested change ∙ Feature Preview
Add a separator between asset and denomination to ensure unique IDs:
function getChainlinkFeedId(event: FeedConfirmedEvent): Bytes {
return event.params.asset.concat(Bytes.fromUTF8('-')).concat(event.params.denomination)
}Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
Description by Korbit AI
What change is being made?
Add a PWN subgraph package with configuration, setup instructions, and ABI contract files.
Why are these changes being made?
These changes provide the necessary implementation to support the PWN subgraph, enabling interaction with smart contracts through the generated subgraph. The inclusion of ABIs and Docker setup facilitates building and running the subgraph efficiently within a development environment.