-
Notifications
You must be signed in to change notification settings - Fork 43
Refactor Dex pool by seperating tick data and position into their own chain objects #564
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
Refactor Dex pool by seperating tick data and position into their own chain objects #564
Conversation
|
|
||
| @BigNumberArrayProperty() | ||
| amounts: BigNumber[]; | ||
| amounts: string[]; |
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 property appears to be missing class-validator decorators that reflect the updated type change
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.
Added validations to this property in the latest commit.
| @IsNotEmpty() | ||
| @ValidateNested() | ||
| @Type(() => PositionsObject) | ||
| positions: PositionsObject; | ||
| positions: IPosition[]; |
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.
Currently this IPosition[] property is only decorated with @IsNotEmpty(). Suggest adding at least one more decorator that is more specific.
I understand if IPosition references an interface, not a Type, it might not work as easily with some of class-validator's options.
But at a minimum I think we could use something like @ArrayNotEmpty() or @ArrayMinSize(0) here in addition to @IsNotEmpty().
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.
Added validations to this property in the latest commit.
| @ChainKey({ position: 2 }) | ||
| @IsInt() | ||
| @Min(TickData.MIN_TICK) | ||
| @IsLessThan("tickUpper") | ||
| tickLower: number; | ||
|
|
||
| @ChainKey({ position: 4 }) |
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.
| @ChainKey({ position: 2 }) | |
| @IsInt() | |
| @Min(TickData.MIN_TICK) | |
| @IsLessThan("tickUpper") | |
| tickLower: number; | |
| @ChainKey({ position: 4 }) | |
| @ChainKey({ position: 2 }) | |
| @IsInt() | |
| @Min(TickData.MIN_TICK) | |
| @IsLessThan("tickUpper") | |
| tickLower: number; | |
| @ChainKey({ position: 3 }) |
Keep the @ChainKey positions sequential
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.
Fixed the skipped chainkey position index.
| @JSONSchema({ | ||
| description: `A tick range mapping that maps a tick range (eg. 10-20) to a unique position ID for this pool` | ||
| }) | ||
| tickRangeMap: Record<string, string[]>; |
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'll want at least some minimum use of validation decorators on tickRangeMap beyond a JSONSchema declaration
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.
Due to the lack of native support in class-validator for validating Record data structures, we’ve implemented a custom validator named IsStringArrayRecord. This custom validation has been applied to the corresponding property to ensure its structure adheres to expected constraints.
sentientforest
left a comment
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.
Good work. The new data structures and reduction in complexity of the Pool class defined in DexV3Pool are steps in the right direction.
No description provided.