-
Notifications
You must be signed in to change notification settings - Fork 0
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
Subgraph setup #1
base: main
Are you sure you want to change the base?
Conversation
14272ee
to
c2aa409
Compare
de337e0
to
def0bcd
Compare
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.
Well done, this isn't a basic subgraph at all! I've just a few comments/questions.
import { ENTRY_POINT } from '../utils/config' | ||
import { ADDRESS_ZERO, BIGINT_ZERO } from '../utils/constants' | ||
|
||
export function handleAccountDeployed(event: AccountDeployed): void { |
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.
At this point we're assuming all Odyssey wallets we'll be created through the Entrypoint
contract (i.e. without calling kernel Factory
directly), is that right?
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.
Correct, I have this setup based few wallets creations that I have observed in Odyssey.
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.
@mbonfoster could you double check it please?
position.openedAt = BIGINT_ZERO | ||
position.txCount = BIGINT_ZERO | ||
position.asset = ADDRESS_ZERO | ||
position.pricePerShare = BIGINT_ZERO |
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 think we could update the isOutdated
field here too. A closed position can be reused (as a way to save gas, avoiding deploying another proxy) so with this flag may trigger an update before re-opening it.
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.
With triggering I mean, bundle [upgrade, open] txs
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.
Good point. I will look into this.
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 here is the deal with this flag. Say we fetch latest status of strategy at the time of close and set it in isOutdated
. Status can be updated between it is closed and opened and there is no way on subgraph to determine this. Only possible solution would be to check isOutdated()
on-chain before reusing position.
We can still set latest at the time of close but it won't be much help IMO.
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.
So here is the deal with this flag. Say we fetch latest status of strategy at the time of close and set it in
isOutdated
. Status can be updated between it is closed and opened and there is no way on subgraph to determine this. Only possible solution would be to checkisOutdated()
on-chain before reusing position. We can still set latest at the time of close but it won't be much help IMO.
Yes, indeed, let's keep it in mind. FYI @mbonfoster
@@ -0,0 +1,3 @@ | |||
import { Address } from '@graphprotocol/graph-ts' | |||
|
|||
export const ENTRY_POINT = Address.fromString('0x0000000071727De22E5E9d8BAf0edAc6f37da032') |
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.
@rokso we expect entry point to change on every new kernel version, and we probably will need to keep support for previous entry points.
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.
Great point. I will think about how we want to handle this.
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.
@patidarmanoj10 cc @marcelomorgado I remember reading somewhere on slack that we can install a module which can emit event at the time of deployment, IMO this can be the best way to handle this. Let me know if this is possible at all.
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.
@mbonfoster when new kernal version is out, which kernel Odyssey will be using for deploying new smart accounts? This setup is only for Odyssey, so we will support whatever version we use on Odyssey side. Can user import smart accounts created outside of Odyssey UI?
Available data points in subgraph
PositionRegistry
SmartAccount
Position
Strategy
PositionDailyData