-
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
Feature/add stake entities and params #26
Feature/add stake entities and params #26
Conversation
…s, added global.ts file to fix an issue caused via the new version of `@subql/types-cosmos` dependency, changed type of computeUnitsPerRelay to BigInt
@bryanchriswhite PTAL when you have a chance. |
…he fields of the entities, changed enums for int, added missing params to block
5838bd8
to
3609a42
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.
LGTM. Great job @Alann27
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 work @Alann27! 🙌 Thanks for all the effort it undoubtedly took to get to this point, I love seeing the indexer come together! 😍 🚀
I'm out of time on this for this morning but will resume review again after I've made some progress on other things:
Some very general feedback: there's waaaay too much going on here for anyone to review as thoroughly as this deserves to be reviewed, especially at this particular stage of the project. High quality review depends on reviewers being able to reason about the entire change holistically. In my view, this perspective is what makes the review context so incredibly useful as compared to the development context. Without clear boundaries between changes though, it becomes extremely difficult to form a holistic picture without risking omitting some relevant detail that's gotten lost in the noise of unrelated changes or being otherwise distracted by unrelated changes.
Going forward, (pretty) please prefer to open several smaller PRs over one large one. It makes review orders of magnitude easier for everyone. 🙏🙏🙏🙏
My number 1 recommendation to facilitate this is to commit related changes frequently. Even if it's not clear from the outset what changes you're going to be making and how it naturally makes sense to organize them, if you prefer many small commits over fewer larger ones, it becomes much more easy to rearrange and squash related changes such that it is then trivial to split the unrelated changes into separate branches, prior to review. If you would like to chat about this in more detail, I would be more than happy to discuss it over a screensharing session or something.
To give you a concrete example, I would've split this PR into the following:
- Update poktroll protobuf files
- Add parameter update indexing
- Add service module indexing
- Add gateway module indexing
- Add application module indexing
- Add supplier module indexing
- Refactor genesis processing
- Add authz message indexing
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.
NO ACTION REQUIRED AT THIS TIME
I think we should start thinking about how to split this file up and reorganize the contents. Additionally, there seems to be another utils.ts file in src/cosmjs. IMHO "utils" files/packages/modules/etc. are generally an antipattern, indicative of a need to reorganize/restructure.
schema.graphql
Outdated
id: ID! | ||
source: Application! | ||
destination: Application! | ||
transaction: Transaction |
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.
transaction: Transaction | |
transaction: Transaction # End-block event; never has tx |
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.
(bump)
schema.graphql
Outdated
source: Application! | ||
destination: Account! | ||
error: String! | ||
transaction: Transaction |
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.
transaction: Transaction | |
transaction: Transaction # End-block event; never has tx |
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.
(bump)
schema.graphql
Outdated
reason: Int! | ||
sessionEndHeight: BigInt! | ||
unstakingEndHeight: BigInt! | ||
transaction: Transaction |
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.
transaction: Transaction | |
transaction: Transaction # End-block event; never has tx |
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.
(bump)
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 to stop again for this morning but this is coming along amazingly! 🙌 Thank you for your persistence and patience. 💪 🙏
ACK re: enum types. We can revisit once SubQuery releases the update that makes GQL enums indexable.
My feedback seems to mostly be about consistency in the following areas:
- Message and event types include relations to all their primitives
- Message and event entity naming (with protobuf types)
- Join table entity naming (i.e.
Entity1Entity2
) - "Similar" field names (e.g. begin/end block/height fields) AND which fields (blocks / heights / both).
schema.graphql
Outdated
id: ID! | ||
source: Application! | ||
destination: Application! | ||
transaction: Transaction |
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.
(bump)
schema.graphql
Outdated
source: Application! | ||
destination: Account! | ||
error: String! | ||
transaction: Transaction |
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.
(bump)
schema.graphql
Outdated
reason: Int! | ||
sessionEndHeight: BigInt! | ||
unstakingEndHeight: BigInt! | ||
transaction: Transaction |
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.
(bump)
service: Service! | ||
revShare: [SupplierRevShare]! | ||
endpoints: [SupplierEndpoint]! |
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.
service: Service! | |
revShare: [SupplierRevShare]! | |
endpoints: [SupplierEndpoint]! | |
serviceConfigs: [SupplierServiceConfig]! |
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.
service: Service! | ||
revShare: [SupplierRevShare]! | ||
endpoints: [SupplierEndpoint]! |
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.
(bump)
Do you disagree here?
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 this may be the last round of feedback - thank you so much for bearing with me! 🙌 🙇♂️
This is great work @Alann27! 🚀 🚀
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 declaration resolves a type mismatch with the `store.getByField` method | ||
// in the generated files. The original method from the library does not | ||
// support a generic type, which causes TypeScript errors when used with generics. | ||
// To fix this, we redefine the `getByField` method to explicitly accept a generic type. |
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.
👌
Summary
The functionality to save the this entities from the genesis were added too.
Details
Service
were added:Application
were added:Application
andGateway
representing the delegation of an application to a gateway.Application
andService
representing the service staked to an application.AppMsgStake
andService
representing the service staked to an application in a specific AppMsgStake.Supplier
were added:SupplierMsgStake
andService
representing the service staked to a supplier in a specific SupplierMsgStake.Supplier
andService
representing the service staked to a supplier.Gateway
were added:To be able to handle the parameters update, we added a new handler called
handleAuthzExec
. This is a message handler that is called when a message is executed by the authz module and inside of it, it has encoded messages likeMsgUpdateParams
. We are decoding thoseMsgUpdateParams
and updating the parameters accordingly.AuthzExec
andMessage
representing the message that was executed by the authz module.Other changes
GeneratedType
inside src/cosmjs directory to add type to the import statement.@subql/node-cosmos
to^4.1.4
. This version includes the fix to the block events not being emitted.@subql/cli
to^5.3.0
and@subql/types-cosmos
to^4.0.0
.getFieldsBy
of store.Issue
@subql/node-cosmos
to^4.1.4
so we can handle the block events.Type of change
Select one or more:
Sanity Checklist