Skip to content
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

Indexing CMM brackets #69

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Indexing CMM brackets #69

wants to merge 4 commits into from

Conversation

josojo
Copy link
Contributor

@josojo josojo commented Aug 6, 2020

This PR makes sure that the basic Fleet deployments are indexed.
A later PR will index more information, which will allow more advanced queries like getting the bracket-strategy price span.

Testplan:

Run through the steps of the Readme

and then check that you see the 1 strategy with 2 brackets deployed on your localhost with this query:

http://127.0.0.1:8000/subgraphs/name/gnosis/protocol/graphql?query=subscription%20UserData%20%7B%0A%20%20fleetDeployeds%20%7Bid%2C%20fleet%7D%0A%7D

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

type FleetDeployed @entity {
id: ID! # txHash + id
owner: Bytes!
fleet: [Bytes!]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably you want to map here an array of other entities, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fleet is really just a gnosis-safe address. At the time of FleetDeployment, there is no additional data available. Only later it would be good to have for each fleet also the orders and balances, but I guess this is done the best within the actual query, as orders and balances are well defined in the graph,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the fleet will have fields in the future (like, orders, trades, balances, and so on) you should model it as an entity in my opinion.

under the hood, the graph saves the fleet id, that can still be the address, but it's just a better approach to try to model your entities since the begining

fleet: [Bytes!]

# Transaction
txHash: Bytes!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look in other entities, it's nice to have audit times too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the deployment of fleet-safe contracts will not be relevant at all. Only the placement of the orders will have any meaning in all use cases, which I can imagine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is not relevant?
is not relevant for the date and tx when it was deployed?

@josojo josojo marked this pull request as ready for review August 12, 2020 07:49
@josojo josojo requested review from anxolin and andre-meyer August 12, 2020 07:49
Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the CMM data requirements (gnosis/safe-cmm-app-react#9), specifically the fact that token pair and amount is needed, I wonder if it would make sense to add that data to the event that gets indexed here (and load from Deposit events).

Or is this very involved and better to do two roundtrips:

  1. to get the tx hash/addresses of the FleetDeployed event
  2. Query deposits of those addresses at the given tx hash.

@josojo
Copy link
Contributor Author

josojo commented Aug 13, 2020

Currently, the fleetdeployment, ordersetting and transferApproveDeposits are happing in a separate transaction and hence, I think they need to be indexed separately and we have to go the roundtrips.

But, @andre-meyer told me yesterday, that this several step process is a huge issue for the gnosis safe. Maybe we would be quicker, if we would do all interaction(fleetdeployment, orderplacement and depositing) in one transaction. I will discuss it with him

@anxolin
Copy link
Contributor

anxolin commented Aug 21, 2020

I'm not super aware of the scripts/model. In my opinion, it will help, both the design and the review, if you can write some UML with the entities and their relations.

I see we are not using the entities we have already defined.
something in the direction of what we have for the other parts:
image

Otherwise, if we see is better to have it separated, we can make it as an independent subgraph

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants