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.

Sign up for GitHub

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

Jump to bottom

Remove CryptoConditionsSuite amendment: #3453

Closed
wants to merge 2 commits into from
Closed

Remove CryptoConditionsSuite amendment: #3453

wants to merge 2 commits into from

Conversation

seelabs
Copy link
Collaborator

@seelabs seelabs commented Jun 15, 2020

  • This was always a stub and enabled no functional changes

@seelabs
Remove CryptoConditionsSuite amendment:
d85d39f 
* This was always a stub and enabled no functional changes
@seelabs seelabs requested review from scottschurr and undertome June 15, 2020 16:04
@seelabs seelabs mentioned this pull request Jun 15, 2020
Closed
@carlhua carlhua linked an issue Jun 15, 2020 that may be closed by this pull request
Remove CryptoConditionsSuite stub amendment #3452
Closed
undertome
undertome approved these changes Jun 15, 2020
View reviewed changes
Copy link
Contributor

@undertome undertome left a comment

Choose a reason for hiding this comment

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

Seems fine.

intelliot
intelliot approved these changes Jun 15, 2020
View reviewed changes
Copy link
Collaborator

@intelliot intelliot left a comment

Choose a reason for hiding this comment

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

It's good to leave the comment to make sure the name is not reused in the future; new amendments should have a new name, e.g. even CryptoConditionsSuiteV2 would be OK since it would (SHA-512Half) hash to a completely different amendment ID.

See suggested change for what looks like a minor typo in the comment.

src/ripple/protocol/impl/Feature.cpp Outdated Show resolved Hide resolved
@carlhua carlhua added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jun 18, 2020
scottschurr
scottschurr approved these changes Jun 18, 2020
View reviewed changes
Copy link
Collaborator

@scottschurr scottschurr left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@mDuo13
Copy link
Collaborator

mDuo13 commented Jun 24, 2020

It's a little weird (and perhaps inadvisable) to think about this way, but it's actually not really a technical problem to reuse the amendment name in the future so long as all servers that had the old version of the amendment are already amendment blocked by something else.

Still probably better for everyone's sanity not to reuse the name, though.

This was referenced Jun 25, 2020
Closed
Closed
Closed
Merged
@seelabs seelabs deleted the rm-cryptocondtions-amendment branch August 10, 2020 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@intelliot intelliot intelliot approved these changes

@mDuo13 mDuo13 mDuo13 approved these changes

@scottschurr scottschurr scottschurr approved these changes

@undertome undertome undertome approved these changes

Assignees

@scottschurr scottschurr

@undertome undertome

Labels
Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove CryptoConditionsSuite stub amendment
6 participants
@seelabs @mDuo13 @intelliot @scottschurr @undertome @carlhua