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

Latest subquery changes #42

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Latest subquery changes #42

wants to merge 20 commits into from

Conversation

Douglasacost
Copy link
Contributor

  • Added compositeIndexes
  • Improved the uniques creation flow
  • Removed Allocation events from mapping
  • Started indexing sponsorship calls

package.json Outdated
"@polkadot/api": "^10",
"@subql/cli": "^4.2.1",
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

@Douglasacost Douglasacost Nov 20, 2023

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

project.yaml Show resolved Hide resolved
project.yaml Show resolved Hide resolved
method: batch
module: sponsorship
method: createPot
- handler: handleSponsorshipRegisterUsersCall
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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 Show resolved Hide resolved
schema.graphql Outdated Show resolved Hide resolved
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"]]) {
Copy link
Contributor

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".

Copy link
Contributor Author

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?

Copy link
Contributor

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 Show resolved Hide resolved
schema.graphql Outdated
Comment on lines 69 to 70
feeQuota: BigInt!
reserveQuota: BigInt!
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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
Comment on lines 56 to 57
feeQuota: BigInt!
reserveQuota: BigInt!
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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

schema.graphql Outdated Show resolved Hide resolved
schema.graphql Outdated Show resolved Hide resolved
schema.graphql Outdated Show resolved Hide resolved
schema.graphql Outdated Show resolved Hide resolved
@aliXsed
Copy link
Contributor

aliXsed commented Nov 21, 2023

@Douglasacost why do we have npm package lock instead of yarn for this project?

@Douglasacost
Copy link
Contributor Author

@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

@Douglasacost
Copy link
Contributor Author

@aliXsed let me know to deploy a Staged node

Comment on lines +6 to +10
'4jbtsgNhpGAzdEGrKRb7g8Mq4ToNUpBVxeye942tWfG3gcYi',
'4jByf7kvkZ7hGYwGMYhjFYHoLec3zNZ3EKD86PARZDcfnnkD',
'1qnJN7FViy3HZaxZK9tGAA71zxHSBeUweirKqCaox4t8GT7',
'4hyBs59AiVKw4jZ851hmEVzpvxFATxwjvTDi67ziwb4vYDCX',
'4j5pigNy7LAX1dSGZQ7Tc2oms9dzZSEtwQJukSgQ9gAsQ9Fx',
Copy link
Contributor

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'
Copy link
Contributor

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?

Comment on lines +28 to +48
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) {
Copy link
Contributor

@aliXsed aliXsed Jan 22, 2024

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

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.

None yet

2 participants