Skip to content

Remove the deadline#9

Open
auryn-macmillan wants to merge 1 commit into
masterfrom
Remove-deadline-validation
Open

Remove the deadline#9
auryn-macmillan wants to merge 1 commit into
masterfrom
Remove-deadline-validation

Conversation

@auryn-macmillan

@auryn-macmillan auryn-macmillan commented Apr 9, 2020

Copy link
Copy Markdown
Member

The deadline validation in the contract serves no purpose, we can validate on the frontend. I have commented it out in this PR.

auryn-macmillan added a commit to burnsignal/burnsignal that referenced this pull request Apr 9, 2020
This is a temporary fix, I also made a [PR in the contract repo](burnsignal/burnSignalContracts#9) to remove the deadline validation from the contracts. I think this can be enforced on the frontend alone.

@Tomasvrba Tomasvrba left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nACK for these changes

  1. While it may serve no purpose in terms of not implementing selfdestruct, it serves other purposes. E.g. We should not allow users to submit/spam proposals that have an end date within seconds or hours etc. It's better to enforce this logic in the trustless backend than in the hosted frontend.
  2. I agree though that the contract could be improved so the frontend can retrieve the deadline from it more easily.
  3. Generally, I prefer to avoid commenting out code, it's always going to be in git history so better to remove it than to have dead code hanging around.
  4. This does not seem worth it going through the contract upgrade process for. This code does not break anything. From a design by contract perspective, the frontend just needs to enforce the deadline rules stipulated by the backend

I'm in favor of closing this PR. But if we decide to remove the deadline validation, the dead code should be removed.

@auryn-macmillan

Copy link
Copy Markdown
Member Author

While it may serve no purpose in terms of not implementing selfdestruct, it serves other purposes. E.g. We should not allow users to submit/spam proposals that have an end date within seconds or hours etc. It's better to enforce this logic in the trustless backend than in the hosted frontend.

I actually disagree with this. Unless the contract is going to revert all incoming funds after the resolution date, I don't think it makes sense for the deadline to have any logic at all. If someone wants to waste gas on a poll that only lasts a few blocks, so be it. We can worry about other spam prevention mechanisms later if it becomes an issue.

I agree though that the contract could be improved so the frontend can retrieve the deadline from it more easily.
👍

Generally, I prefer to avoid commenting out code, it's always going to be in git history so better to remove it than to have dead code hanging around.

Great point. Will keep that in mind for future PRs.

This does not seem worth it going through the contract upgrade process for. This code does not break anything. From a design by contract perspective, the frontend just needs to enforce the deadline rules stipulated by the backend

Yeah, I do tend to agree with this. But I would like to roll this, or something similar, into whatever the next upgrade is.

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 this pull request may close these issues.

2 participants