1

I've built a contract that stores an IPFS link and maps it to a uint256. It's not an ERC721 contract just a regular one. It keeps all the data in a mapping (mapping(uint256 => Data)), but the issue is querying that data. Right now, the only function I have to return all data created by an address is to iterate over the whole mapping, locally storing the data that matches that address, and returning it. This isn't effective as the time to return will grow linearly with the amount of data that is stored.

I have thought of a solution with would include nested mappings. It would look something like this: mapping(address => mapping(uint256 => Data)). I implemented this and tested it with Hardhat but the time it takes actually increases...

I was wondering if it wasn't the way of storing the values but maybe the actual functions that find the data aren't optimized, but I'm not that good of a Solidity dev to figure that one out...

I'll show stripped-down code but will also post the full code on Github.


Here are the functions I've been testing:

Old Contract: link

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/utils/Counters.sol";

contract TestHub {
  using Counters for Counters.Counter;
  Counters.Counter private _noteIds;

  event NoteCreated(
    uint256 indexed noteId,
    address indexed owner,
    bytes32 contentId
  );
  
  struct Note {
    uint256 noteId;
    address owner;
    bytes32 contentId;
  }

  mapping(uint256 => Note) private noteRegistry;
  
  function createNote(
    string calldata _contentUri
  ) external {
    _noteIds.increment();

    address _owner = msg.sender;
    uint256 _noteId = _noteIds.current();

    noteRegistry[_noteId] = Note(
      _noteId,
      _owner,
      _contentId
    );

    emit NoteCreated(_noteId, _owner, _contentId);
  }

  function getNote(uint256 _noteId) public view returns (Note memory) {
    address _owner = msg.sender;

    Note memory noteToCheck = noteRegistry[_noteId];
    Note memory noteToReturn;

    if (noteToCheck.owner == _owner) {
      noteToReturn = noteToCheck;
    }

    return noteToReturn;
  }

  function getNotes() public view returns (Note[] memory) {
    address _owner = msg.sender;
    uint256 totalNotesToQuery = _noteIds.current();
    uint256 noteCount = 0;
    uint256 currentIndex = 0;

    for (uint256 i = 0; i < totalNotesToQuery; i++) {
      if (noteRegistry[i + 1].owner == _owner) {
        noteCount += 1;
      }
    }

    Note[] memory notes = new Note[](noteCount);

    for (uint256 i = 0; i < totalNotesToQuery; i++) {
      if (noteRegistry[i + 1].owner == _owner) {
        Note storage currentNote = noteRegistry[i + 1];
        notes[currentIndex] = currentNote;
        currentIndex += 1;
      }
    }

    return notes;
  }
}

New Contract: link

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/utils/Counters.sol";

contract Hub {
  using Counters for Counters.Counter;
  Counters.Counter private _noteIds;

  event NoteCreated(
    uint256 indexed noteId,
    address indexed owner,
    bytes32 contentId
  );
  
  struct Note {
    uint256 noteId;
    address owner;
    bytes32 contentId;
  }

  mapping(address => mapping(uint256 => Note)) private noteRegistry;
  
  function createNote(
    string calldata _contentUri
  ) external {
    _noteIds.increment();

    address _owner = msg.sender;
    uint256 _noteId = _noteIds.current();

    noteRegistry[_owner][_noteId] = Note(
      _noteId,
      _owner,
      _contentId
    );
    
    emit NoteCreated(_noteId, _owner, _contentId);
  }

  function getNote(uint256 _noteId) public view returns (Note memory) {
    address _owner = msg.sender;

    Note memory noteToCheck = noteRegistry[_owner][_noteId];
    Note memory noteToReturn;

    if (noteToCheck.owner == _owner) {
      noteToReturn = noteToCheck;
    }

    return noteToReturn;
  }

  function getNotes() public view returns (Note[] memory) {
    address _owner = msg.sender;
    uint256 totalNotesToQuery = _noteIds.current();
    uint256 noteCount = 0;
    uint256 currentIndex = 0;

    for (uint256 i = 0; i < totalNotesToQuery; i++) {
      if (noteRegistry[_owner][i + 1].owner == _owner) {
        noteCount += 1;
      }
    }

    Note[] memory notes = new Note[](noteCount);

    for (uint256 i = 0; i < totalNotesToQuery; i++) {
      if (noteRegistry[_owner][i + 1].owner == _owner) {
        Note storage currentNote = noteRegistry[_owner][i + 1];
        notes[currentIndex] = currentNote;
        currentIndex += 1;
      }
    }

    return notes;
  }
}


Update

Would someone explain why this method of storing data is slower?

mapping(address => mapping(uint256 => Data))

Surely instead of looping over every item in the array like I was previously doing should be slower than this? With this method you first get the list by the address which is accessed using msg.sender, and then you loop over the items that address has made. So why is it slower?

1 Answer 1

1

First of your 2 problems is that you are iterating twice over the same array. To ditch the first iteration altogether you might create a second mapping which holds the current number of notes associated with specific address. Now you have noteCount without iterating.

mapping(address => uint256) addressToNumberOfNotes;

Second iteration could be improved too (talking about your new improved contract). Do you really need to check the owner on every note? Your structure guarantees this check passes every time.

Also theres this global counter that makes id of every note unique. Do you really need this or is it sufficient to have unique ids only in every user scope?

1) I need to have globally uniqueId

Then I would create yet another mapping that links users index to globalIndex. Now with the help of the addressToNumberOfNotes you can get specific globally uniqueid of a specific user.

Edit: users index just increases with each time user stores a new note. (last users index == addressToNumberOfNotes[usersAddress])

mapping(address => mapping(uint256 => uint256)) addressToNoteId;

2) I just need the id to be unique in user scope

Then you don't need the counter at all and just use the addressToNumberOfNotes to get the latest id.

Either way mapping through the entire array can be unbearably expensive in terms of GAS and makes your contract vulnerable to attacks.

Edit: Provided you don't need to have a globally unique noteId, I've put together this improved solution:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

contract Hub {
    event NoteCreated(
        uint256 indexed noteId,
        address indexed owner,
        bytes32 contentId
    );

    struct Note {
        uint256 noteId;
        address owner;
        bytes32 contentId;
    }
    mapping(address => mapping(uint256 => Note)) private noteRegistry;
    mapping(address => uint256) private addressToNumberOfNotes;

    function createNote(string calldata _contentUri) external {
        address _owner = msg.sender;
        uint256 _noteId = addressToNumberOfNotes[_owner] + 1;
        bytes32 _contentId = keccak256(abi.encode(_contentUri));

        addressToNumberOfNotes[_owner] = _noteId;
        noteRegistry[_owner][_noteId] = Note(_noteId, _owner, _contentId);

        emit NoteCreated(_noteId, _owner, _contentId);
    }

    function getNote(uint256 _noteId) public view returns (Note memory) {
        address _owner = msg.sender;

        Note memory note = noteRegistry[_owner][_noteId];

        require(
            note.owner == _owner,
            "You can't get notes that doesn't belong to you!"
        );

        return note;
    }

    function getNotes() public view returns (Note[] memory) {
        address _owner = msg.sender;

        Note[] memory notes = new Note[](addressToNumberOfNotes[_owner]);

        for (uint256 i = 0; i < addressToNumberOfNotes[_owner]; i++) {
            Note storage currentNote = noteRegistry[_owner][i];
            notes[i] = currentNote;
        }

        return notes;
    }
}

Haven't tested that, there might be some indexing issues, but will work much better. Also added a require() statement to your getNote() as a bonus

7
  • Thanks so much for this answer it clears a lot of questions up. I've removed the first loop in the function and that makes a 10% reduction, which is very good. I got a bit confused on how to integrate the second bullet point. Could you show me it in action in Solidity? Also, I updated my post which may be a better way... Jan 29 at 15:38
  • Sorry, can't comment on your post so I'll do it here: Regarding to your updated question, the reason why it's slower is pretty obvious from your code: Look at how you're accessing the mapping: for (uint256 i = 0; i < totalNotesToQuery; i++) { if (noteRegistry[_owner][i + 1].owner == _owner) { Note storage currentNote = noteRegistry[_owner][i + 1]; notes[currentIndex] = currentNote; currentIndex += 1; } } Jan 29 at 15:46
  • Oh yeah ok my bad haha.... Would I have to create yet another counter to keep track of the total number of owners? And in regards to the note counter, is there a way to remove this? because I don't need the ID unique over all users, just unique to their respective address. Jan 29 at 15:48
  • You are still iterating totalNotesToQuery number of times, even though the individual address sub-mappings have way less notes. The way that mapping works is when accessing index that hasn't been initialized yet, it'll return base implementation of Note and won't go through your owner check there. The reason why it's even slower is that now you nested the mapping, so the additional time is the first search for an address Jan 29 at 15:50
  • Gimme a minute I'll post the concrete implementation now knowing that you don't need to have globally unique id Jan 29 at 15:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.