-
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
Latest subquery changes #42
base: main
Are you sure you want to change the base?
Conversation
Douglasacost
commented
Nov 18, 2023
- Added compositeIndexes
- Improved the uniques creation flow
- Removed Allocation events from mapping
- Started indexing sponsorship calls
docker-compose.yml
mappingAllocationHandler files
Sponsorship
package.json
Outdated
"@polkadot/api": "^10", | ||
"@subql/cli": "^4.2.1", |
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.
Was there and issue to use "latest" 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.
On another note, I would say using an actual version could be a better choice than "latest". So you may want to use the version uniformly for all of "subql" dependencies.
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.
Yes, had some issues with the package manager Yarn
Indeed, I already had issues related to it, that time only with @subql/cli
method: batch | ||
module: sponsorship | ||
method: createPot | ||
- handler: handleSponsorshipRegisterUsersCall |
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.
Usually the EventHandlers are a better choice for indexing compared with the CallHandlers. The reason is when the call is nested for example inside a Utility batch, the subquery used to struggle to trace it. Is it still the case?
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.
True @aliXsed, we had issues with the batch in the past since it handle tons of data (e.x allocations)
Will update this on testnet, for use events instead of calls
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.
Will prepare a new PR when the sponsorship events got updated on mainnet, meanwhile pot indexing is desactivted
schema.graphql
Outdated
@@ -24,7 +24,7 @@ type Item @entity { | |||
owner: String | |||
} | |||
|
|||
type BalanceTransfer @entity { | |||
type BalanceTransfer @entity @compositeIndexes(fields: [["from", "to", "amount"], ["from", "to", "timestamp"], ["from", "to"]]) { |
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 composite indexing doesn't seem to be able to accelerate queries such as from not equal to allocations to dao or company reserve. Correct me if I'm wrong. I'm not against keeping it if it's making our other usual queries faster. However to satisfy https://linear.app/nodle/issue/CHA-137 you may need to create a new entity such as
type TransferToTreasury @entity {
id : ID!
from: String! @index
to: String! @index # This field is still needed because we have a number of treasury accounts and not just one
timestamp: Float
blockNumber: Int
}
The other option is you add another field to BalanceTransfer
such as nonRewardToTreasury: Boolean @index
to make it clear which entities are "transfers to one of our treasuries but not coming from the allocations".
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.
@aliXsed could you give me all the treasury accounts, or it is a constant in the chain?
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.
Company Reserve: 4jbtsgNhpGB2vH7xTjpZVzZLy7W4sFyxjvD45x7X1m6BSiGx
International Reserve: 4jbtsgNhpGB2voF5dZzKQ2tphWLjV48HSkfQwmWqNn3qa4rv
USA Reserve: 4jbtsgNhpGB2voKv8rRSJYTAohbnHE5oVZ4DejZBEQVT5o86
DAO Reserve: 4jbtsgNhpGB2NtbdLN3xkB2urHo5JboKkSAjcBM4s3SzQdUt
Allocations: 4jbtsgNhpGAzdEGrKRb7g8Mq4ToNUpBVxeye942tWfG3gcYi
schema.graphql
Outdated
feeQuota: BigInt! | ||
reserveQuota: BigInt! |
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.
Quota
should be able to show two fields: limit
and balance
.
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.
Currently is showing the available feeQuota & reserveQuota
available = limit - balance
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.
we'd want to know both the used quota and the available quota about the users. So perhaps it's worth adding both.
schema.graphql
Outdated
feeQuota: BigInt! | ||
reserveQuota: BigInt! |
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.
Quota
should be able to show two fields: limit
and balance
.
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.
Currently is showing the available feeQuota & reserveQuota
available = limit - balance
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 imagine we'd want to know both the used quota and the available quota about the users. So perhaps it's worth adding both.
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.
Update to registry Limit and Balance for feeQuota and reserveQuota
@Douglasacost why do we have npm package lock instead of yarn for this project? |
Subquery team recommend use NPM, that's why I started using it |
@aliXsed let me know to deploy a Staged node |
'4jbtsgNhpGAzdEGrKRb7g8Mq4ToNUpBVxeye942tWfG3gcYi', | ||
'4jByf7kvkZ7hGYwGMYhjFYHoLec3zNZ3EKD86PARZDcfnnkD', | ||
'1qnJN7FViy3HZaxZK9tGAA71zxHSBeUweirKqCaox4t8GT7', | ||
'4hyBs59AiVKw4jZ851hmEVzpvxFATxwjvTDi67ziwb4vYDCX', | ||
'4j5pigNy7LAX1dSGZQ7Tc2oms9dzZSEtwQJukSgQ9gAsQ9Fx', |
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.
Can you remind where these addresses come from? you can also put a comment in front of each to remain here.
'4j5pigNy7LAX1dSGZQ7Tc2oms9dzZSEtwQJukSgQ9gAsQ9Fx', | ||
] | ||
|
||
const ALLOCATION_ACCOUNT = '4jbtsgNhpGAzdEGrKRb7g8Mq4ToNUpBVxeye942tWfG3gcYi' |
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 account has become part of both Rewards and Treasury accounts. What is the logic for that?
if (isReward) { | ||
result.push(new Rewards( | ||
`${event.block.block.header.number.toNumber()}-${event.idx}`, | ||
'', | ||
'' | ||
)) | ||
} | ||
|
||
const amount = event.event.data[2]; | ||
let record = new BalanceTransfer(`${event.block.block.header.number.toNumber()}-${event.idx}`, '', ''); | ||
record.blockNumber = event.block.block.header.number.toBigInt(); | ||
record.from = from.toString(); | ||
record.to = to.toString(); | ||
record.amount = (amount as Balance).toBigInt(); | ||
if (event.extrinsic) { | ||
record.txHash = event.extrinsic.extrinsic.hash.toString(); | ||
record.timestamp = event.extrinsic.block.timestamp.getTime(); | ||
record.success = checkIfExtrinsicExecuteSuccess(event.extrinsic) | ||
|
||
if (isTreasury) { | ||
const entity = new TransferToTreasury( | ||
`${event.block.block.header.number.toNumber()}-${event.idx}`, | ||
'', | ||
'' | ||
) | ||
|
||
entity.isAllocation = from.toString() === ALLOCATION_ACCOUNT | ||
|
||
result.push(entity) | ||
} | ||
|
||
if (!isReward && !isTreasury) { |
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 suggest a switch-case or a complete if-else structure for the logic here to make sure we actually separate:
- Treasury Commission from Rewards
- Rewards
- Refund to Treasury
- User to User Transfers
into their own dedicated tables with no overlap. As a result, the return from this function should also change from an array of entities to a single entity
…nto feat/composed-index