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

Proposal: Tables should expose an encodeKey() view #751

Closed
yonadaa opened this issue May 8, 2023 · 1 comment · Fixed by #753
Closed

Proposal: Tables should expose an encodeKey() view #751

yonadaa opened this issue May 8, 2023 · 1 comment · Fixed by #753

Comments

@yonadaa
Copy link
Contributor

yonadaa commented May 8, 2023

As part of the Factories PR (#749), I need to generically pass around table keys as an array of bytes32. This is difficult because the logic for serialising keys is inlined in table contracts:

 /** Get the full data (using the specified store) */
  function get(...) internal view returns (StaticsData memory _table) {
    bytes32[] memory _primaryKeys = new bytes32[](7);
    _primaryKeys[0] = bytes32(uint256((k1)));
    _primaryKeys[1] = bytes32(uint256(uint32((k2))));
    _primaryKeys[2] = bytes32((k3));
    _primaryKeys[3] = bytes32(bytes20((k4)));
    _primaryKeys[4] = _boolToBytes32((k5));
    _primaryKeys[5] = bytes32(uint256(uint8(k6)));
    _primaryKeys[6] = bytes32(uint256(uint8(k7)));

    bytes memory _blob = _store.getRecord(_tableId, _primaryKeys, getSchema());
    return decode(_blob);
  }

Like the encode() method for data, this logic should be moved to a encodeKey() method like:


  function encodeKey(...) internal pure returns (bytes32[] memory) {
    bytes32[] memory _primaryKeys = new bytes32[](7);
    _primaryKeys[0] = bytes32(uint256((k1)));
    _primaryKeys[1] = bytes32(uint256(uint32((k2))));
    _primaryKeys[2] = bytes32((k3));
    _primaryKeys[3] = bytes32(bytes20((k4)));
    _primaryKeys[4] = _boolToBytes32((k5));
    _primaryKeys[5] = bytes32(uint256(uint8(k6)));
    _primaryKeys[6] = bytes32(uint256(uint8(k7)));

    return _primaryKeys;
  }

I experimentally added this functionality here but it should be moved to a separate PR.

@dk1a
Copy link
Contributor

dk1a commented May 8, 2023

I agree, the rendering logic is already encapsulated in _primaryKeysDefinition, I haven't made a separate method only because it hasn't been needed yet

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 a pull request may close this issue.

2 participants