-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add support for forced request approvals #137
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
feat: add support for forced request approvals #137
Conversation
packages/utils/src/components/request_approvals/request_approvals.cairo
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #137 +/- ##
=======================================
Coverage ? 89.36%
=======================================
Files ? 43
Lines ? 2464
Branches ? 0
=======================================
Hits ? 2202
Misses ? 262
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dc9069e to
45de586
Compare
packages/utils/src/components/request_approvals/request_approvals.cairo
Outdated
Show resolved
Hide resolved
45de586 to
5bc5900
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.
@remollemo reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @ishay-starkware)
packages/utils/src/components/request_approvals/request_approvals.cairo line 19 at r3 (raw file):
pub struct Storage { approved_requests: Map<HashType, RequestStatus>, forced_approved_requests: Map<HashType, (Timestamp, RequestStatus)>,
Suggestion:
forced_action_requestspackages/utils/src/components/request_approvals/request_approvals.cairo line 47 at r3 (raw file):
/// The signature is verified with the hash of the request. /// The request is stored with a status of PENDING. fn register_approval<T, +OffchainMessageHash<T>, +Drop<T>>(
-
consolidate
fn register_approval
with the inner part that is now calledfn unsefe_register_approval -
rename
fn unsafe_register_approvalto something more sane. maybe
fn _store_approval
Code quote:
fn register_approval<T, +OffchainMessageHash<T>, +Drop<T>>(packages/utils/src/components/request_approvals/request_approvals.cairo line 72 at r3 (raw file):
/// is validated with signature and caller checks via `register_forced_approval`, while /// this function is used to register the original request hash in the approvals map. fn unsafe_register_approval<T, +OffchainMessageHash<T>, +Drop<T>>(
this name is bad. it's not unsafe at all. it's simply an unvalidated internal flow.
see comment above
update commment accordingly
Code quote:
fn unsafe_register_approvapackages/utils/src/components/request_approvals/request_approvals.cairo line 87 at r3 (raw file):
/// The approval is signed with the public key. /// The signature is verified with the hash of the request. /// The request is stored with a status of PENDING, and the current time.
how is it expeted to be used? (it's an intenal one, it's called from within fn forced_withdraw for example?
Suggestion:
/// Registers a forced action.
/// If the owner_account is non-zero, the caller must be the owner_account.
/// The request is signed with the public key.
/// The request is stored with a status of PENDING, and the current time.
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 1 files reviewed, 5 unresolved discussions (waiting on @ishay-starkware and @MohammadNassar1)
a discussion (no related file):
it's very unclear how you bind the action_request with the forced_action_request.
5bc5900 to
71e6a79
Compare
MohammadNassar1
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 1 files reviewed, 4 unresolved discussions (waiting on @ishay-starkware and @remollemo)
a discussion (no related file):
Previously, remollemo wrote…
it's very unclear how you bind the action_request with the forced_action_request.
The forced request stores a hash computed from the same fields as a regular request. in addition, both have similar logic (calculating hash then validating sigs, and storing).
So the link between a regular request and its forced counterpart is that they share the same request fields. The forced map just adds timestamp + its own status while the regular map tracks the generic approval status.
I added comments, so the link between the request would be clearer.
packages/utils/src/components/request_approvals/request_approvals.cairo line 47 at r3 (raw file):
Previously, remollemo wrote…
consolidate
fn register_approval
with the inner part that is now calledfn unsefe_register_approvalrename
fn unsafe_register_approvalto something more sane. maybe
fn _store_approval
Done.
packages/utils/src/components/request_approvals/request_approvals.cairo line 87 at r3 (raw file):
Previously, remollemo wrote…
how is it expeted to be used? (it's an intenal one, it's called from within
fn forced_withdrawfor example?
Exactly, it's called in forced_withdraw_request and force_trade_request.
packages/utils/src/components/request_approvals/request_approvals.cairo line 19 at r3 (raw file):
pub struct Storage { approved_requests: Map<HashType, RequestStatus>, forced_approved_requests: Map<HashType, (Timestamp, RequestStatus)>,
Done.
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 1 files reviewed, 1 unresolved discussion (waiting on @ishay-starkware)
packages/utils/src/components/request_approvals/request_approvals.cairo line 73 at r4 (raw file):
) -> HashType { self._store_approval(:public_key, :args) }
this makes no sense.
delete fn store_approval() and call fn _store_approval() directly.
Code quote:
fn store_approval<T, +OffchainMessageHash<T>, +Drop<T>>(
ref self: ComponentState<TContractState>, public_key: PublicKey, args: T,
) -> HashType {
self._store_approval(:public_key, :args)
}
MohammadNassar1
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 1 files reviewed, 1 unresolved discussion (waiting on @ishay-starkware and @remollemo)
packages/utils/src/components/request_approvals/request_approvals.cairo line 73 at r4 (raw file):
Previously, remollemo wrote…
this makes no sense.
deletefn store_approval()and callfn _store_approval()directly.
_store_approval is a private function, so it cannot be called from withdraw contract.
and since I call it in register_approval, I think we should have a private method.
71e6a79 to
a355d5d
Compare
MohammadNassar1
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 1 files reviewed, 1 unresolved discussion (waiting on @ishay-starkware and @remollemo)
packages/utils/src/components/request_approvals/request_approvals.cairo line 73 at r4 (raw file):
Previously, MohammadNassar1 (mohammad-starkware) wrote…
_store_approvalis a private function, so it cannot be called from withdraw contract.
and since I call it inregister_approval, I think we should have a private method.
Removed _store_approval.
| self.approved_requests.write(key: request_hash, value: RequestStatus::PENDING); | ||
| self | ||
| .forced_action_requests | ||
| .write(key: request_hash, value: (Time::now(), RequestStatus::PENDING)); |
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: Forced approval misses required dual-map registration
The register_forced_approval function only writes to forced_action_requests but doesn't register the request in approved_requests. According to the documentation for store_approval, forced requests should be tracked in both maps to maintain consistency. This breaks the dual-map invariant and could cause issues when the request is later processed, as _get_request_status() will return NOT_REGISTERED instead of PENDING.
Note
Introduce timestamped forced-approval flow with new storage and APIs, and refactor approval registration to a helper.
forced_action_requestsmap storing(Timestamp, RequestStatus)perrequest_hash.register_forced_approval(...)to register forced actions withTime::now()and PENDING status.consume_forced_approved_request(...)to mark forced actions PROCESSED and return(timestamp, request_hash).store_approval(...); makeregister_approval(...)delegate to it and returnHashType._get_forced_action_request_status(...).Written by Cursor Bugbot for commit a355d5d. This will update automatically on new commits. Configure here.
This change is