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

Retrieve functions do not check expiryTime #50

Open
mpeyfuss opened this issue Dec 26, 2023 · 3 comments
Open

Retrieve functions do not check expiryTime #50

mpeyfuss opened this issue Dec 26, 2023 · 3 comments

Comments

@mpeyfuss
Copy link

Problem Statement

Most, if not all, retrieve functions do not check the expiry time of a delegation. You can see in this example function (retrieveGlobalStatusOfDelegation) that there is no check of expiry time. A global delegation may be expired, but this function will return back true as the length of the array is greater than 0.

function retrieveGlobalStatusOfDelegation(address _delegatorAddress, address _collectionAddress, address _delegationAddress, uint256 _useCase) public view returns (bool) {

This can lead to confusion for developers trying to use this delegation registry, especially if they aren't fluent in Solidity. Perhaps this stems from my expectations based on the description of the retrieve functions, but in my opinion, the code should make it as simple as possible and not rely on logic to be implemented elsewhere. This registry should act as the source of truth any time I call it and not rely on logic to be implemented elsewhere. If I were going to check these functions in another contract, I want the truth returned to me so I can use it for my own purposes.

@mpeyfuss
Copy link
Author

Something else that is likely important is that retrieveDelegators does not check expiry times which could allow for expired sub-delegators to act after their time has expired.

@a2rocket
Copy link
Collaborator

Taking into account the expiry date of a delegation clearly depends on the developer who will integrate the smart contract. For this purpose we have developed a specific retrieve function namely, retrieveActiveDelegators(...) that takes into consideration a specific epoch time.

By executing this function you can retrieve only the active delegators for a specific delegation address based on the expiry date.

@mpeyfuss
Copy link
Author

If the contract defines an expiry time, shouldn't that be enforced everywhere? Seems like an easy way to lead to improper integration with third party apps. At least naming the functions something like retrieveAllHistoricalDelegates would give a clue to devs on what is returned.

Additionally, better documentation + natspec comments on these considerations would be incredibly helpful. Right now, devs just have to guess at the purpose of a function based on one comment in the source code and aren't aware of any pitfalls, such as not checking expiry time.

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

No branches or pull requests

2 participants