-
Notifications
You must be signed in to change notification settings - Fork 27
Rest api for google sheets #158
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Overall great job, this certainly accomplishes what we need. I've left a bunch of low level comments on the code, with the only important one being that currently if you don't pass an authorization header you can just get admin access. Some high level discussion + comments:
General approach
Setting up a proxy API server was certainly a viable decision, but it's not the only one. The most obvious alternative is to add these endpoints to the existing rust server, which happens to work similar to flask except the one endpoint it currently has happens to be a websocket endpoint, allowing a persistent bi-directional channel with the client. The implementation I had in mind as taking 1-2 days was to embed javascript on the google sheet using a custom menu instead of appsscript, which would allow to create a regular connection to the server like the frontend does and update the google sheet live. I think in the future we'll want to transition to the live updating version, at which point we can decide if to keep the rest api for other purposes.
Tech choices
- uv for dependency management would have been nice, it solves a bunch of problems and is already being used in the codebase.
- Flask is kind of legacy at this point IMO, and I don't like it very much. I get why an llm recommended it though, it's was the standard for python api servers for a long time. I prefer FastAPI, which is the library we're using for scenarios that auto-generates the nice interactive API docs.
- Overall a pretty easy way to set this up would have been to copy over the stuff in the scenarios repo and delete the anything that's not needed.
Obscure fears
I'm a bit concerned that if we give the students a refresh button on the spreadsheet we'll hit the appsscript rate limit and everything will stop working for everyone, so maybe we should be cautious about that somehow. Also I have a suspicion that with all the students having cached clients we'll run out of the 512mb of memory once there are bots involved.
LMK if you want me to elaborate / dive deeper on anything!
middleware/middleware.py
Outdated
| return client # fallback to default client | ||
|
|
||
| try: | ||
| jwt, act_as = auth_header.split("|") |
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.
This is a non standard header format, which might make it weird for people to try using the rest api not through the google sheet. I'd probably put the act as in a query parameters (it can be left out - act_as is allowed to just be 0 signifying 'act as myself'), and the standard format for the authorization header is Bearer <jwt>, which flask probably has a way of parsing for you.
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.
Actually, I think you probably just don't want them to pass act_as at all since they're not sending orders anyway. They'll get visibility of all of their owned (recursive) accounts whether they act_as or not
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.
let me know if the current one is better?
middleware/middleware.py
Outdated
| trades = [ | ||
| { | ||
| "id": trade.id, | ||
| "market_id": trade.market_id, | ||
| "transaction_id": trade.transaction_id, | ||
| "price": trade.price, | ||
| "size": trade.size, | ||
| "buyer_id": trade.buyer_id, | ||
| "seller_id": trade.seller_id, | ||
| } | ||
| for trade in state.markets[market_id].trades | ||
| ] | ||
| return jsonify(trades) |
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 general rule you want to return an object (dict) as the top level of the response rather than an array. This is more self describing, and it lets you add more fields to the response later without breaking existing clients. So something like (pseudo code):
{
trades: [
...
]
}
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 made a change that just takes the trades directly and JSONify's them directly
trades = state.markets[market_id].trades
Is this fine? It ends up outputting things like this (this one is for orders but you can image the same concept for trades):

If not, I think I don't fully understand the syntax, can you explain again? Is something being assigned as a valuable to that object? I'm getting issues about trades being unbound
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.
This output looks fine, to understand the issue you're describing I'd have to see the code
|
FYI, this is where the spreadsheet currently lives: https://docs.google.com/spreadsheets/d/1QQ7GVo9KDLbJduu7iQZwSSDVAOTNCJBrsRLTNlw0SKc/edit?gid=0#gid=0 And this is the code I have in the apps script: |
|
@Crazytieguy I think I addressed most of your comments. The things that didn't work the way I'd hoped:
|
|
Small apps script review I think you're reading from J3, but asking to write to A1? Also you can give the cell a name and then refer to it by name. It looks like you're catching the thrown exception just to immediately throw a basically identical exception. I think you don't need the try / catch at all. The rest seems good I think |
|
I made the switch to uv, but I wasn't able to verify that it works correctly because the docker registry is having a production incident (rare!). I think you accidentally deleted the first line in the pyproject.toml file and maybe that caused you problems |
|
I also renamed middleware to rest proxy (but I left the fly app name since you've already deployed it). BTW you deployed it to your personal fly.io I think, so if you want anyone else to have access to the deployment you'll have to add us as collaborators to your fly account, or you can delete that deployment and we can deploy it from my account |
|
Hm, I tried the deleting and redeploying from the organization approach, and something seems to not be working. I'm not entirely sure how to best approach testing it. Let me know if you have free time at some point to walk me through that process? |
|
I tested the dockerfile and fixed it, if that was your issue it should be good now. Otherwise I'm available rn, you can catch me on discord |
I'd like to be able to use Google Sheets. I know this doesn't feel top priority but I am unsatisfied with the current state of scenarios monitoring, and I want to be able to just use Google Sheets, so I'm doing it.