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

Added at_height and absolute_timelocks #253

Merged
merged 2 commits into from
Aug 13, 2021

Conversation

SarcasticNastik
Copy link
Contributor

Related to #247 and #248 . Added the at_height function for analyzing policies and absolute_timelocks for semantic analysis.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

Some general comments about the PR:

In general, usually, PRs are rebased on top of the latest master to avoid conflicts. Looks like you are merging the master branch onto the PR branch. 1. Read https://www.atlassian.com/git/tutorials/merging-vs-rebasing for starters.
2. All commits in the PR should compile and pass the test. This helps tools like git bisect for easier debugging. The second commit in this PR
37a2ad28636fb4694672db88c0efebab393f8375 has incorrect tests which are later fixed in a follow commit.
3. The unused variable was introduced in the first commit and later fixed in the third commit. We should just not introduce it in the first commit.

@sanket1729
Copy link
Member

The CI is broken right now, once #250 is fixed. I will add CI enforcement.

@SarcasticNastik
Copy link
Contributor Author

SarcasticNastik commented Jul 16, 2021

Thanks for the suggestions. I've updated my master with the commits as suggested and have rebase-d it.

sanket1729
sanket1729 previously approved these changes Jul 16, 2021
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

tACK bfdc790.

Copy link
Contributor

@darosior darosior left a comment

Choose a reason for hiding this comment

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

ACK bfdc790 -- modulo the API nit

Comment on lines 502 to 503
/// Helper function for recursion in `absolute timelocks`
pub fn real_absolute_timelocks(&self) -> Vec<u32> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: if this is a helper function, we'd rather not make it public?

Copy link
Member

Choose a reason for hiding this comment

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

Ideally, this should be been fixed in cac7815 and not in the next commit, but it' okay :)

@darosior
Copy link
Contributor

darosior commented Aug 4, 2021

ACK 1091d07

It's better for 1091d07 to be squashed in bfdc790 in order to keep the changes atomic, up to the maintainers tho and that can be considered a nit :)

@sanket1729
Copy link
Member

@SarcasticNastik, normally I wouldn't be so picky, but it would be a good exercise for you to try. Can you please make the change that Darosior requested?

@SarcasticNastik
Copy link
Contributor Author

I've made the commit changes suggested by @darosior and squashed them into 925ae34.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK 925ae34

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.

3 participants