GalaChain / sdk

GalaChain SDK allows you to develop, test, call and deploy chaincodes on GalaChain
Apache License 2.0
120 stars 29 forks source link

`FetchAllowances` should include `grantedBy`, when provided, in the range query #381

Open sentientforest opened 20 hours ago

sentientforest commented 20 hours ago

Given the following:

https://github.com/GalaChain/sdk/blob/main/chaincode/src/allowances/fetchAllowances.ts#L32-L55

// This will return the list sorted by creation date descending (oldest first)
// If the "From Filter" is applied, it will filter the results
export async function fetchAllowances(
  ctx: GalaChainContext,
  data: FetchAllowancesParams
): Promise<TokenAllowance[]> {
  const queryParams: string[] = takeUntilUndefined(
    data.grantedTo,
    data.collection,
    data.category,
    data.type,
    data.additionalKey,
    data.instance,
    data.allowanceType?.toString()
  );

  const getObjectsResponse = await getObjectsByPartialCompositeKey(
    ctx,
    TokenAllowance.INDEX_KEY,
    queryParams,
    TokenAllowance
  );

  const results = filterByGrantedBy(getObjectsResponse, data.grantedBy);

https://github.com/GalaChain/sdk/blob/main/chain-api/src/types/TokenAllowance.ts#L68-L71

  @ChainKey({ position: 6 })
  @EnumProperty(AllowanceType)
  public allowanceType: AllowanceType;

  // This would make it hard to find all allowances issued out...
  @ChainKey({ position: 7 })
  @IsUserAlias()
  public grantedBy: string;

When we encounter a hypothetical scenario such as:

  1. Some game has a centralized / authoritative "game admin" type account
  2. Users grant some kind of allowance to said game admin, e.g. AllowanceType.Burn
  3. Regular game activity causes concurrent fetch/lookup and use of these allowances across transactions within the same block

Then we will see PHANTOM_READ conflicts due to:

Even if all ChainKeys, 1 through 7, are provided to the FetchAllowances method, from owner through to grantedBy, the range queries' specificity will still only be limited to the allowanceType and it will filter by grantedBy after the fact.

So if multiple users are granting some allowance to a game admin, and then attempting to fetch/use their own specific allowances concurrently, the read set on their individual transactions will include a range read with all other users grantedBy values, even though their write set only includes their own individual TokenAllowance write.

Propose adding some small logic such that

a) grantedBy is included in the takeUntilUndefined() query and b) filterByGrantedBy is not executed if all keys up to and including grantedBy were previously provided to the getObjectsByPartialCompositeKey() query.

mistval commented 18 hours ago

Would the fix have any effect on FetchAllowances requests where the only filter is grantedTo?

sentientforest commented 18 hours ago

@mistval - no, requests that leave out other TokenAllowance ChainKeys, like additionalKey, instance, allowanceType, would not be affected. They would still execute a range query with a larger results set and then filter by grantedTo after the fact.

sentientforest commented 18 hours ago

Fix here: https://github.com/GalaChain/sdk/pull/382

Existing tests pass. But our current tests don't have a straightforward way to test two transactions executing in the same block so for expediency I have not added new tests with the above.

sentientforest commented 15 hours ago

Finished some updates to the public SDK's e2e test facilities. Replicated bug and fix with a new e2e test in chaincode-template:

https://github.com/GalaChain/sdk/pull/383