-
Notifications
You must be signed in to change notification settings - Fork 5
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
CERT 8125 Support Multisig Address Check #83
Conversation
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.
Looks great.
if not response.ok: | ||
return None | ||
data = response.json() | ||
return MultisigData(**data) |
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.
Can we ensure data format wont change?
Do we want to try/catch here or we don't expect this to happen?
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.
Can we ensure data format wont change?
Not really it is dependant on their API
Do we want to try/catch here or we don't expect this to happen?
It would be better to get the validation error and adjust the API to make sure we return the necessary data
@@ -106,14 +109,20 @@ def verify_price_feed(self) -> None: | |||
elif res := self.__check_address(address, self.token_providers): | |||
verified_variables.append(res.price_feed.model_dump()) | |||
verified_tokens.add(res) | |||
elif res := self.multisig_api.get_multisig_info(address): |
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.
So our only way to understand if this is multisig, is to try and fetch the data from SafeAPI, and in case of failures/success we move forward?
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.
Yep, if the API return a valid obj then this address is a multisig else its not
from quorum.apis.multisig.safe_api import SafeAPI | ||
|
||
|
||
def test_safe_api(): |
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.
As a continuation for the comment above, can we verify that the SafeAPI return None for non-multisig addresses?
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.
Done
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.
Straight forward implementation, mainly some questions or small improvements.
Please add another small test case as described in comments.
f37f111
https://certora.atlassian.net/browse/CERT-8125
PR Message
Summary:
This PR introduces multisig support by adding a new module in src/quorum/apis/multisig/ with a SafeAPI class to interact with the Safe Multisig API.
The multisig integration is incorporated into the PriceFeedCheck class so that multisig wallet data is verified alongside other price feed providers.
A new test (test_mutisig_api.py) has been added to ensure that multisig data is correctly retrieved and parsed.
Key Changes:
Added src/quorum/apis/multisig/safe_api.py with a MultisigData model and SafeAPI class (using a singleton decorator).
Modified src/quorum/checks/price_feed.py to:
Instantiate SafeAPI and query multisig data for addresses.
Append multisig results to the list of verified variables.
Print a multisig-specific summary of validation results.