-
Notifications
You must be signed in to change notification settings - Fork 1
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
Modify PRE operator schema's entity #11
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import { Address } from "@graphprotocol/graph-ts" | ||
import { SimplePREApplication } from "../../generated/schema" | ||
|
||
export function getOrCreatePreApplication( | ||
stakingProvider: Address | ||
): SimplePREApplication { | ||
const preApplication = SimplePREApplication.load( | ||
stakingProvider.toHexString() | ||
) | ||
|
||
return !preApplication | ||
? new SimplePREApplication(stakingProvider.toHexString()) | ||
: preApplication | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
export * from "./delegations" | ||
export * from "./epochs" | ||
export * from "./applications" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 ID should be the operator address, not the staking provider because we are in the PREOperator entity and we should add a relationship between
PREOperator
andStakeData
entity.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.
That's a good point. I agree with ID being the operator's address.
But I don't see clearly how to add a relationship between PREOperator and StakeData. Do you mean a Reverse Lookup? This will imply to add a new field in StakeData (probably called preoperator), and in my opinion, we shouldn't mix Threshold Network Staking (StakeData) with an Threshold application of the network (PRE).
Or maybe you mean to use One-To-Many relationship? In that case, do you agree with this schema?
The PREOperator entity will include an array of all stakeDatas that use this operator.
What do you think?
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 makes sense. But it should probably be a one-to-one relationship because of this requirement https://github.com/nucypher/nucypher-contracts/blob/main/contracts/contracts/SimplePREApplication.sol#L193 cc @cygnusv.
Another solution could be a schema like this:
So in that case we are ready for multi-staking app authorization and the
applications
field makes sense in theStakeData
entity because we authorize applications to use a given stake. What do you think? @manumonti @cygnusv ofc we can address it in a separate PR just a thought.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 find your proposal very reasonable and I mostly agree with that.
Only one comment about this:
I think it could be easier to implement this if
id
isstakingProviderAdress-preapplication
. I understand that this entity isPREApplication
so ID should includeoperator address
, but there is a case which makes hard to implement this:Check this line: https://github.com/nucypher/nucypher-contracts/blob/710b68713b5b8fece0d0b1c5de376dba1d7d68e7/contracts/contracts/SimplePREApplication.sol#L206
If I am not wrong, this means that when an operator is unbonded, an
operatorBonded
event is emitted with these parameters:stakingProvider
= staking provider addressoperator
= 0x00000000000000000(please @cygnusv , @vzotova can you confirm?)
So, if the ID is composed of the
operator
address, we can't easily find thePREOperator
entity in order to modify or delete it when this type of event is received.What do you think, @r-czajkowski ?
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.
Yeah, that's right and it works for me. I'm curious about David's opinion.
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.
Unbonding operator will lead to
operator = 0x00000000000000000
in an eventThere 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.
On the other hand, this is probably a rare scenario (but I'm not sure) and there will be 3 apps (at least for now: tbtc, random beacon, and pre) so I don't mind iterating through
applications
from theStakeData
entity and finding the app which contains-preapplication
in the ID and then removing it from the store. Just a thought. Both solutions work for me 👍.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.
Hi @r-czajkowski. Although I think your proposal scheme fits for Threshold Applications (PREApplication, RandomBeacon, TBTC...), IMHO it can't be applied to
SimplePREApplication
because:It doesn't make sense to have an
authorizedStakeAmount
field inSimplePREApplication
since events for authorized stake are not emitted yet.There is a discussion in progress about these events: threshold-network/solidity-contracts#88 (comment).
I have thought about calling the
authorizedStake
function from SimplePREApplication contract (https://github.com/nucypher/nucypher-contracts/blob/710b68713b5b8fece0d0b1c5de376dba1d7d68e7/contracts/contracts/SimplePREApplication.sol#L95-L101), but the state of this field won't be synced since no new events are emitted if stake amounts are modified.In any case, about this:
As I understand, this can't work in your proposal schema since the applications field is a derived field. And it is not possible to fetch derived fields in the mappings.ts, since these are built at query time. See https://github.com/graphprotocol/graph-ts/issues/219.
In my opinion, we should create a new issue or PR with your proposed schema and address it when a definitive version of some Threshold application (PREApplication, TBTC, RandomBeacon) is deployed.
Please, let me know your opinion about 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.
I finally created a issue addressing this. So if @r-czajkowski agree, we can continue the conversation there.
#23