-
Notifications
You must be signed in to change notification settings - Fork 10
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
Adds API support for Dropper v0.2.0 #275
Conversation
Introduced the `ENGINE_DEV` environment variable to make it easier to run the Engine API in development environment. To use this, set: ``` export ENGINE_DEV=true ```
Also added a `create_account_signer` method in `signatures.py`.
Want to add moonstream_user_id to registered contracts so that users can add their own contracts.
In database schema, we added moonstream_user_id to registered_contracts table (and regenerated alembic migration). Also fixed CLI commands for `engineapi engine-db contracts` to respect the `moonstream_user_id`. The goal is to allow Moonstream users to register contracts that they want self-serve, without stepping on anyone else's toes.
Better db_session handling on errors (proper rollbacks) Better output form CLI (JSON).
And alembic migration
`contracts.py` -> `contracts_actions.py`
This code was generated in a pair programming session with ChatGPT. This is how ChatGPT summarized our work: - We started by discussing your preferences for software development, including your high-level architectural preferences. - You showed me some code you were working on and we discussed how to add a new command to the CLI. We added a new command to the CLI, called request-calls, that invokes the request_calls method with arguments provided by the user. - We implemented the request_calls method, which takes call specifications and stores them in the database for later execution. - We implemented the handle_request_calls method, which parses the call specifications from a file provided by the user and passes them to request_calls. - We tested the new functionality through the CLI invocation. - Finally, we made some improvements to the error handling in handle_request_calls.
From `List[Tuple[str, str, Dict[str, Any]]]` to `List[data.CallSpecification]`. Internal items are expected to be JSON objects with structure: ``` { "caller": "<caller address>", "method": "<method name>", "parameters": {...} } ```
Open question re: indexing.
@bugout-dev check
|
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.
sm thoughts
class RegisteredContract(Base): # type: ignore | ||
__tablename__ = "registered_contracts" | ||
__table_args__ = ( | ||
UniqueConstraint( |
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.
We need to check everywhere in actions that contract_type
and blockchain
pass Enum, otherwise database could have Ethereum
or ETHEREUM
values and UniqueConstraint will not affect.
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 now, this is being validated at CLI level and at API level (FastAPI handler parses the type). I could mark the inputs to the API and CLI as strings and do the parsing explicitly in the actions. Would you prefer that?
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.
Added TODO regarding this in the code.
image_uri: Optional[str] = None | ||
|
||
|
||
class RegisteredContract(BaseModel): |
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.
If you will add
class Config:
orm_mode = True
there would no need in function render_registered_contract
it will automaticaly handle parsing from sqlalchemy to pydantic.
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.
It's a good suggestion. I will make this update after first deployment - since it's a bit urgent today.
def sign_handler(args: argparse.Namespace) -> None: | ||
# Prompt user to enter the password for their signing account | ||
password_raw = input(f"Enter password for signing account ({args.signer}): ") | ||
password = password_raw.strip() |
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.
Why we stopped using signatures here, for example I would prefer to sign message on local machine and pass it on production if required admin access?
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 don't understand the question. We didn't stop using signatures - the schema for dropper-v0.2.0
requires a signature.
# Calculate the expiration time (if ttl_days is specified) | ||
expires_at_sql = None | ||
if ttl_days is not None: | ||
expires_at_sql = text(f"(NOW() + INTERVAL '{ttl_days} days')") |
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.
Better not to use user input with raw queries and pass it through sqlalchemt query builder, or at least have a check this is real integer.
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 have created a TODO for this. I agree this is a bad pattern (came from ChatGPT). Need to figure out how to do this correctly with SQLAlchemy.
- Defensive exception raise in contract_type validator - Made title non-nullable for registered_contracts
Based on review by @kompotkot and @Andrei-Dolgolev
As per @Andrei-Dolgolev feedback on PR.
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.
lg
Changes
This pull request introduces the
/contracts
API which allows Moonstream users to:This is a generalization of the metatransaction workflow implemented currently by the
/dropper
API.Currently, the
/contracts
API supports requesting users to claim tokens from Dropper v0.2.0 as well as requesting users to submit arbitrary transactions against any smart contract (using theraw
contract type).It also includes a CLI available under
engineapi engine-db contracts
which administrators can use to manage registered contracts and contract call requests.How to test these changes?
These changes were tested manually against a development instance of the Engine API.
Related issues
Resolves #273