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

Support merge cells' data. #189

Closed
yanguoyu opened this issue Mar 14, 2023 · 12 comments
Closed

Support merge cells' data. #189

yanguoyu opened this issue Mar 14, 2023 · 12 comments
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@yanguoyu
Copy link
Contributor

For merging the cells' data in Store, I think we should consider the following things.

  1. Provider default merge functions to merge cells, use the latest cell's data, or merge all cells' data by lodash.merge
  2. When calling get, developers can get with merged data or appointed OutputPoint
  3. When calling set, developers can merge all cells or update appointed OutputPoint

In my mind, here is my design to achive these:

  1. Add a merge cells' data property mergeState in Store
class Store<
  StorageT extends ChainStorage,
  StructSchema extends StorageSchema<GetState<StorageT>>,
  Option = never,
> extends Actor<GetStorageStruct<StructSchema>, MessagePayload<StoreMessage>> {
  protected mergeState: GetStorageStruct<StructSchema>
}
  1. Provide a function to merge cells' data that developers can overwrite.
class Store<
  StorageT extends ChainStorage,
  StructSchema extends StorageSchema<GetState<StorageT>>,
  Option = never,
> extends Actor<GetStorageStruct<StructSchema>, MessagePayload<StoreMessage>> {
    useLatest: boolean = false
    mergeCellsData(current: GetStorageStruct<StructSchema>, last: GetStorageStruct<StructSchema>): StructSchema {
        if (this.useLatest) {
           return current
        }
        const customizer = <T>(objValue: T, srcValue: T) => {
          if (Array.isArray(objValue)) {
            return objValue.concat(srcValue)
          }
        }
    
        return mergeWith(current, last, customizer)
    }
}
  1. update mergeState when addState or removeState
  private addState({ cell, witness }: UpdateStorageValue) {
    if (this.cellPattern && !this.cellPattern({ cell, witness })) {
      // ignore cells from resource binding if pattern is not matched
      return
    }
    if (cell.outPoint) {
      const outPoint = outPointToOutPointString(cell.outPoint)
      const value = this.deserializeCell({ cell, witness })
      if (this.schemaPattern && !this.schemaPattern(value)) {
        return
      }
      this.states[outPoint] = value
      this.chainData[outPoint] = { cell, witness }
      this.mergeState = this.mergeCellsData(value, this.mergeState)
    }
  }
  private removeState(keys: OutPointString[]) {
    keys.forEach((key) => {
      delete this.states[key]
      delete this.chainData[key]
    })
   Object.values(this.states).reduce((pre, cur) => this.mergeCellsData(pre, cur), {})
  }
  1. get and set support calls without delivering OuputPoint
get(paths?: StorePath): GetFullStorageStruct<StructSchema> {
     // get value from this.mergeState
}
set(value: any, paths?: StorePath) {
      if (this.useLatest) {
      // get the latest cell to update and add capacity from old cells
      return
      }
      //if update all, merge all cells to a new cell
      // if updating one key, find the cell and use updated data, if capacity is not enough maybe use other cells.
}
@yanguoyu yanguoyu added the documentation Improvements or additions to documentation label Mar 14, 2023
@yanguoyu yanguoyu self-assigned this Mar 14, 2023
@Keith-CY
Copy link
Member

Please have a review @zhengjianhui @PainterPuppets @Daryl-L

@Keith-CY
Copy link
Member

Keith-CY commented Mar 14, 2023

For the set method

When calling set, developers can merge all cells or update appointed OutputPoint

What about adding a new strategy:

set(path, value) will locate the cell holding the path and update the specific cell, e.g.

Store:
  field_a in cell_a
  field_b in cell_b

Store.get() => { field_a, field_b }
Store.set(field_a, new_value) => only update cell_a

If a path exists in multiple cells, only one cell will be updated

@yanguoyu
Copy link
Contributor Author

set(value: any, paths?: StorePath) {
      if (this.useLatest) {
      // get the latest cell to update and add capacity from old cells
      return
      }
      //if update all, merge all cells to a new cell
      // if updating one key, find the cell and use updated data, if capacity is not enough maybe use other cells.
}

Of cause, from this code

set(value: any, paths?: StorePath) {
      if (this.useLatest) {
      // get the latest cell to update and add capacity from old cells
      return
      }
      //if update all, merge all cells to a new cell
      // if updating one key, find the cell and use updated data, if capacity is not enough maybe use other cells.
}

There are three situations:

  1. Developers set useLatest to true, it will get the latest cell to update
  2. Developer doesn't deliver the path, it will merge all cells into a new cell
  3. Developer delivers the path, it will find the cell and update the data.

@Keith-CY
Copy link
Member

Keith-CY commented Mar 15, 2023

set(value: any, paths?: StorePath) {
      if (this.useLatest) {
      // get the latest cell to update and add capacity from old cells
      return
      }
      //if update all, merge all cells to a new cell
      // if updating one key, find the cell and use updated data, if capacity is not enough maybe use other cells.
}

Of cause, from this code

set(value: any, paths?: StorePath) {
      if (this.useLatest) {
      // get the latest cell to update and add capacity from old cells
      return
      }
      //if update all, merge all cells to a new cell
      // if updating one key, find the cell and use updated data, if capacity is not enough maybe use other cells.
}

There are three situations:

  1. Developers set useLatest to true, it will get the latest cell to update
  2. Developer doesn't deliver the path, it will merge all cells into a new cell
  3. Developer delivers the path, it will find the cell and update the data.

For the situation 2, does it mean set(value: any) merges all cells into one with value stored in it, and all data from various cells are overridden?

@yanguoyu
Copy link
Contributor Author

There are three situations:

  1. Developers set useLatest to true, it will get the latest cell to update
  2. Developer doesn't deliver the path, it will merge all cells into a new cell
  3. Developer delivers the path, it will find the cell and update the data.

For the situation 2, does it mean set(value: any) merges all cells into one with value stored in it, and all data from various cells are overridden?

yes.

@yanguoyu
Copy link
Contributor Author

yanguoyu commented Mar 24, 2023

I have a problem when finding the cell to change when call set. Here is the test case:

const store = new JSONStore<{ data: { a: string, b?: BigNumber, c?: { d: string } } }>({ data: true })
const outPointTmp = { txHash: `0x01${'0'.repeat(62)}`, index: '0x0'}
const updates = [
  createUpdateParams({ data: store.initOnChain({ data: { a: 'a1', c: { d: 'd1' } }}).data, outPoint: defaultOutpoint }),
  createUpdateParams({ data: store.initOnChain({ data: { a: 'a2' }}).data, outPoint: outPointTmp })
]
store.handleCall(updates[0])
store.handleCall(updates[1])
const changedStore = store.set(['data', 'c', 'd'], 'd2')

From the cells' order, before calling set, the mergedState is { data: { a: 'a2', c: { d: 'd1' } } }
After I call the set, I should set the defaultOutpoint.c.d to 'd2', but I also need to remove the defaultOutpoint.a, because the mergedState.a is from outPointTmp.

Now my problem is how to find the keys that need to remove to update the cell's state. I have an idea to achieve it:
When I merge the cells' state, I will mark all the paths' OutPoint. So when I find the cell that needs to change, I will check that the path in the found cell but the path's OutPoint does not belong to the found cell, and remove the paths.
Here is the code:

// used to save the mergedState's value exist in which cells
protected mergeStatesLocation: Record<OutPointString, string[]> = {}
private mergeCellsData(current, last, outPointString) {
  const useCurrentPaths = []
  // mergeWith need to achieve by myself that can get current value's full path
  const res = mergeWith((obj, src, paths) => {
    if (Array.isArray(obj)) {
      useCurrentPaths.push(paths)
      return obj.concat(src)
    }
    if (isSimpleType(obj) && src !== undefined) {
       useCurrentPaths.push(paths)
    }
  }, last, current)
  this.mergeStatesLocation[outPointString] = useCurrentPaths
  return res
}
set(value: any, paths?: StorePath) {
     const outPointString = this.findByPaths(paths)
     const allPaths = getAllPaths(this.states[outPointString])
     // remove paths that are in allPaths but not in this.mergeStatesLocation[outPointString]
}

But it seems too complex, do you have any suggestions to resolve it? @Keith-CY @PainterPuppets @Daryl-L @zhengjianhui

@yanguoyu
Copy link
Contributor Author

Maybe there exists another way to resolve it, when I will update the cell by calling set, I can replace all the values on this cell from the mergedState.

set(value: any, paths?: StorePath) {
     const outPointString = this.findByPaths(paths)
     // it will ergodic
     deepForIn(this.states[outPointString])((value, keys) => {
         if(isSimpleType(value) && get(this.mergedState, keys)) {
            set(this.states[outPointString], keys, get(this.mergedState, key))
         }
     })
     set(this.states[outPointString], paths, value)
}

@Keith-CY
Copy link
Member

Keith-CY commented Mar 24, 2023

I didn't get the point of

From the cells' order, before calling set, the mergedState is { data: { a: 'a2', c: { d: 'd1' } } }
After I call the set, I should set the defaultOutpoint.c.d to 'd2', but I also need to remove the defaultOutpoint.a, because the mergedState.a is from outPointTmp.

Why should defaultOutpoint.a be removed. IMO, the internal states should be

defaultOutpoint: { a: 'a1', c: { d: 'd2' } }
outPointTmp: { a: 'a2' }

And the state to outsides is

{ a: 'a2', c: { d: 'd2' } }

{ a: 'a1' } is still in the internal state but overridden by { a: 'a' }

The reason is that { a: 'a1' } is useful in some cases

Say we change the merge strategy to combine instead of override

internal_state_1: { a: 'a1' }
internal_state_2: { a: 'a2' }
=>
external_state: { a: ['a1', 'a2'] }

'a1' should not be eliminated

@yanguoyu
Copy link
Contributor Author

I didn't get the point of

From the cells' order, before calling set, the mergedState is { data: { a: 'a2', c: { d: 'd1' } } }
After I call the set, I should set the defaultOutpoint.c.d to 'd2', but I also need to remove the defaultOutpoint.a, because the mergedState.a is from outPointTmp.

Why should defaultOutpoint.a be removed. IMO, the internal states should be

defaultOutpoint: { a: 'a1', c: { d: 'd2' } }
outPointTmp: { a: 'a2' }

And the state to outsides is

{ a: 'a2', c: { d: 'd2' } }

{ a: 'a1' } is still in the internal state but overridden by { a: 'a' }

Of cause, before the state sends to the blockchain, the state is still { a: 'a2', c: { d: 'd2' } }, but if developers broadcast a tx from the changed state.

inputs: defaultOutpoint.cell
outputs: defaultOutpoint.cell after set to { a: 'a1', c: { d: 'd2' } }

Then after the tx success, the merged state will be { a: 'a1', c: { d: 'd2' } }, because store receive a new cell that it states is { a: 'a2', c: { d: 'd2' } }.

@yanguoyu
Copy link
Contributor Author

The reason is that { a: 'a1' } is useful in some cases

Say we change the merge strategy to combine instead of override

internal_state_1: { a: 'a1' }
internal_state_2: { a: 'a2' }
=>
external_state: { a: ['a1', 'a2'] }

'a1' should not be eliminated

And if we should not remove { a: 'a1' } , I think this #189 (comment) is effective

@Keith-CY
Copy link
Member

I didn't get the point of

From the cells' order, before calling set, the mergedState is { data: { a: 'a2', c: { d: 'd1' } } }
After I call the set, I should set the defaultOutpoint.c.d to 'd2', but I also need to remove the defaultOutpoint.a, because the mergedState.a is from outPointTmp.

Why should defaultOutpoint.a be removed. IMO, the internal states should be

defaultOutpoint: { a: 'a1', c: { d: 'd2' } }
outPointTmp: { a: 'a2' }

And the state to outsides is

{ a: 'a2', c: { d: 'd2' } }

{ a: 'a1' } is still in the internal state but overridden by { a: 'a' }

Of cause, before the state sends to the blockchain, the state is still { a: 'a2', c: { d: 'd2' } }, but if developers broadcast a tx from the changed state.

inputs: defaultOutpoint.cell
outputs: defaultOutpoint.cell after set to { a: 'a1', c: { d: 'd2' } }

Then after the tx success, the merged state will be { a: 'a1', c: { d: 'd2' } }, because store receive a new cell that it states is { a: 'a2', c: { d: 'd2' } }.

Got it, any idea from @PainterPuppets @zhengjianhui @Daryl-L

@Keith-CY
Copy link
Member

If no other feedback on the question above, the strategy to override the duplicated key will be adopted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Archived in project
Development

No branches or pull requests

2 participants