Zellic audit #2 feedback #199
Conversation
…ulatable with small exercise() calls
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #199 +/- ##
==========================================
+ Coverage 91.22% 99.20% +7.97%
==========================================
Files 2 1 -1
Lines 376 250 -126
Branches 57 41 -16
==========================================
- Hits 343 248 -95
+ Misses 27 0 -27
+ Partials 6 2 -4
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
…e; Bump copyright year; Fix comment typos
| emit OptionsWritten(encodedOptionId, msg.sender, tokenId, amount); | ||
| emit BucketWrittenInto(encodedOptionId, tokenId, bucketIndex, amount); |
There was a problem hiding this comment.
is this part of the feedback? I recall discussing but can't recall outcome of the discussion re:exposing the bucket concept via events. Feel free to resolve if we've already had this talk. but first take is that this seems orthogonal to the offered feedback and also seems like it exposes internals unnecessarily. again, could be forgetting a discussion we had
There was a problem hiding this comment.
Yeah, we needed it for constructing claim state on the subgraph
| amountPresentlyExercised = amount; | ||
| amount = 0; | ||
| } | ||
| bucketInfo.amountExercised += amountPresentlyExercised; | ||
|
|
||
| emit BucketAssignedExercise(optionId, bucketIndex, amountPresentlyExercised); |
There was a problem hiding this comment.
looks like this is the line that addresses offline accounting
There was a problem hiding this comment.
Right, but without the BucketWrittenInto event, an off-chain actor won't be able to make heads or tails of what this event means. They need to know how many options, for each claim/index, are being written into the buckets to support off-chain claim accounting.
There was a problem hiding this comment.
@neodaoist please review the comment I left in the impl file; feel free to resolve it if we've already had that discussion.
other than that, it seems like the description of the PR is slightly off; the seed update is not included in the delta for this PR
They should be. |
|
settlementSeedupdating in_assignExercise()function_assignExercise()function signature, to pass inoptionIdparameter)feeBpsfrom immutable to constant, suppressingconst-name-snakecasesolhint warning (https://github.com/valorem-labs-inc/valorem-core/pull/201/files#diff-033680dc4cc50d115cdec7fe97c8be2fcf62aacef3c4b50b7c9979c08ad0027fR102)OptionSettlementEnginetoValoremOptionsClearinghouseIValoremOptionsClearinghouseinterfaceNote: you can press
ato toggle check annotations on/off