-
Notifications
You must be signed in to change notification settings - Fork 6
feat: opt risk factor #81
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
base: dev
Are you sure you want to change the base?
Conversation
d3bc482 to
69289b1
Compare
cc7e83b to
ca18c61
Compare
ca18c61 to
5043da9
Compare
19b6d86 to
e0bd885
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.
Bug: Timestamp Overwrite Causes Inconsistent Validation
The _validate_order function recomputes the now timestamp on line 1306, overwriting the value initially set from the optional parameter. This defeats the optimization to reuse a consistent timestamp across multiple order validations, leading to redundant Time::now() calls and potentially inconsistent expiration checks.
workspace/apps/perpetuals/contracts/src/core/core.cairo#L1305-L1306
starknet-perpetual/workspace/apps/perpetuals/contracts/src/core/core.cairo
Lines 1305 to 1306 in e0bd885
| // Expiration check. | |
| let now = Time::now(); |
d9699ee to
b9373a3
Compare
| self.assets.get_collateral_id() | ||
| } else { | ||
| collateral_id.unwrap() | ||
| }; |
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.
Bug: Redundant Time::now() Call Breaks Timestamp Consistency
Duplicate Time::now() call in _validate_order function. The now variable is already computed at lines 1108-1112, but line 1127 calls Time::now() again, overwriting the parameter value. This defeats the purpose of passing now as an optional parameter for reuse between order_a and order_b validation, potentially causing inconsistent timestamp checks.
| self | ||
| .risk_factor_tiers_opt | ||
| .entry(asset_id) | ||
| .read(index.try_into().expect('INDEX_SHOULD_NEVER_OVERFLOW')) |
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.
Bug: New Assets Lack Risk Config Initialization
get_asset_risk_factor reads from risk_config and risk_factor_tiers_opt storage fields, but the _add_asset function (which adds new assets) does not populate these fields. Only the migrate_risk function populates them. This means newly added assets will have default/zero values for risk configuration, causing incorrect risk factor calculations until migrate_risk is manually called. The _add_asset function should populate these new storage fields when adding an asset.
b9373a3 to
fecd82d
Compare
remollemo
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.
Reviewable status: 0 of 7 files reviewed, 8 unresolved discussions (waiting on @RoeeGross)
workspace/apps/perpetuals/contracts/src/core/types/asset.cairo line 117 at r9 (raw file):
} /// ┌──────────────────────────────────────────────────────────────┐
almost good
make really good
workspace/apps/perpetuals/contracts/src/core/components/assets/assets.cairo line 179 at r9 (raw file):
let value = vec.at(i.into()).read(); self.risk_factor_tiers_opt.entry(asset_id).write(i.into(), value); }
rethink, redo
- not in the same place
- consider alternative to map
Code quote:
let vec = self.risk_factor_tiers.entry(asset_id);
for i in 0..vec.len() {
let value = vec.at(i.into()).read();
self.risk_factor_tiers_opt.entry(asset_id).write(i.into(), value);
}
Note
Packs risk config into optimized storage and migrates data, updates risk-factor lookup, refactors trade order validation reuse, and adjusts tests/interface.
RiskConfigwithStorePackingand new storage:risk_configandrisk_factor_tiers_opt.migrate_risk()to populate new structures from existingasset_configandrisk_factor_tiers.get_asset_risk_factorto userisk_configandrisk_factor_tiers_opt.read._validate_orderto accept optionalnow/collateral_idand return them; reuse in_validate_trade._validate_trade(commented out).RiskConfigstruct andStorePacking<RiskConfig, felt252>implementation.migrate_riskinIAssetsand call it in performance test beforemulti_trade.Written by Cursor Bugbot for commit fecd82d. This will update automatically on new commits. Configure here.
This change is