6529-Collections / nftdelegation

Delegation contract
MIT License
16 stars 7 forks source link

updateDelegationAddress will always fail when updating expiry date #13

Closed brookr closed 1 year ago

brookr commented 1 year ago

The updateDelegationAddress function calls registerDelegationAddress before it calls revokeDelegationAddress. But within the registerDelegationAddress function, there is a line that reads:

require(registeredDelegation[globalHash] ==false);

Because the expiry date is not a part of the globalHash, this appears to mean that if a user attempts to update a delegation with a new expiry date for, say, the Minting use case, it will be rejected. The requirement will find there there is already a registered delegation with the existing From, Collection, To, and Use Case values.

I suspect this could be resolved by calling revokeDelegationAddress before calling registerDelegationAddress within the updateDelegationAddress function.

Alternatively: Don't provide an updateDelegationAddress function at all. Users will have to update with the 2 steps of revoking, then creating a new delegation.

rookmate commented 1 year ago

You can submitt a PR solving this issue instead of having it as an issue no?

brookr commented 1 year ago

Indeed, PR linked right below the comment. I generally start with creating an issue, to see if there is a discussion worth having before making modifications to the code. In this case, it was a small change, so kicked off the issue, then went on immediately to make the PR.

brookr commented 1 year ago

Even though the PR wasn't merged, this change looks to have been accepted into version 4.33 released recently. @skyn3t2030, feel free to close this issue, unless you'd like to keep it open for any further discussion.

skyn3t2030 commented 1 year ago

thanks for your comments guys!