-
Notifications
You must be signed in to change notification settings - Fork 12
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
Support off-chain claim accounting via bucket events #185
Conversation
@@ -598,7 +600,7 @@ contract OptionSettlementEngine is ERC1155, IOptionSettlementEngine { | |||
address underlyingAsset = optionRecord.underlyingAsset; | |||
|
|||
// Assign exercise to writers. | |||
_assignExercise(optionTypeState, optionRecord, amount); | |||
_assignExercise(optionId, optionTypeState, optionRecord, amount); |
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.
BTW, with the entropy + weighting changes, it's likely that we can drop optionRecord from the function params.
amountPresentlyExercised = amount; | ||
amount = 0; | ||
} | ||
bucketInfo.amountExercised += amountPresentlyExercised; | ||
|
||
emit BucketAssignedExercise(optionId, bucketIndex, amountPresentlyExercised); |
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.
I believe amountPresentlyExercised
is the correct argument to include here — the amount of options that were just assigned exercised from this bucket.
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.
One sm0l change @neodaoist
* @param bucketIndex The index of the bucket to which the options were written. | ||
* @param amount The amount of options contracts written. | ||
*/ | ||
event BucketWrittenInto(uint256 indexed optionId, uint96 bucketIndex, uint112 amount); |
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.
I think this should have the claim ID indexed for the subgraph handler to know.
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.
RIght on, good call, will add
Proposed changes in this PR —
BucketWrittenInto
eventBucketAssignedExercise
eventWill save for the more significant entropy and weighting PRs —