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

Fix lockup issues found by @ailisp #153

Merged
merged 11 commits into from
Jul 6, 2021
Merged

Conversation

telezhnaya
Copy link
Contributor

The first commit solves #145
The second commit adds the instruction on how to build the contract.

All the other commits are addressing issues in the report file-by-file, you could check them separately if you want to.

I also partially fixed issues from #146, some of them were intersected with the issues from the report.


What haven't been done during this PR:

  • I didn't fix the comments in README. You could see some fixes there because they are required to have a consistent state with the code, but I will re-check the comments from @ailisp separately
  • I didn't fix Lockups: confusing doc about termination information #139 because I don't feel confident in it. @ailisp does not find the problem there at all. I feel I need more time to dig into that, not sure the issue worth it.
  • I didn't mark as deprecated transfers_information, voting, and other related stuff. While I was rewriting README on the previous iteration, @evgenykuzyakov pointed out that it's not correct to say that transfers were enabled on Oct 13, 2020, because the contract could be invoked not only in the Mainnet, and that date could differ. So, from that point of view, the mechanism should not also be deprecated, it still could be used.
  • I didn't rename foundation_account_id to terminator_account_id because it changes the public interface, so it could break the backward compatibility.

@AngelBlock you could be also interested in it

Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

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

Lgtm! Nice job

@telezhnaya telezhnaya force-pushed the olya/fix-lockup-after-review branch from 9329c8c to c996969 Compare July 5, 2021 13:54
@telezhnaya
Copy link
Contributor Author

Rebased because we merged required PR (nothing is changed in this PR)
Now diff is exactly as it should be

@telezhnaya telezhnaya merged commit 0e220b2 into master Jul 6, 2021
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.

Lockups: confusing doc about termination information
3 participants