-
Notifications
You must be signed in to change notification settings - Fork 29
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
Only ascii alphanumeric strings in tokens after v1 #1283
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides that cleanup requirement, this is looking OK.
6bcfada
to
442debb
Compare
e061194
to
0a4b7a4
Compare
Please don't merge this until I slap another approval. I still have to review it again. I have some doubts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to ensure it won't be merged by mistake.
9768935
to
6cd5f00
Compare
0a4b7a4
to
4604434
Compare
let current_height = self | ||
.get_gen_block_index(&prev_block_id)? | ||
.ok_or(CheckBlockTransactionsError::PropertyQueryError( | ||
PropertyQueryError::BlockIndexNotFound(prev_block_id), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use the error PrevBlockIndexNotFound. Also, this is not a nonchalante error... this is an invariant broken.
Why not use the function that was made for this anyway? get_previous_block_index_for_check_block()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use the error PrevBlockIndexNotFound.
Agree, done.
Why not use the function that was made for this anyway?
It returns incompatible Error type. It doesn't do much so I think it's ok to leave it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides that minor thing, I think this is fine.
4604434
to
d6b167f
Compare
No description provided.