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] #3853: Compare permission token payload as JSON #3855

Conversation

mversic
Copy link
Contributor

@mversic mversic commented Aug 31, 2023

Description

  1. StringWithJson should implement PartialEq comparison as JSON
    this was causing bugs when iroha-java would serialize token payload vs when iroha serde would do so
  2. remove DefaultValidator struct from iroha_validator
    it's been used incorrectly, because the delegated method must always call back into original validator whereas calling into DefaultValidator would prevent this. Interestingly, removing DefaultValidator didn't increase code duplication which was the argument for introducing it in the first place
  3. removed DoesAccountHavePermissionToken
    As is discussed in [BUG] Eq/Ord for PermissionToken implementations are broken #3857 the comparison of PermissionTokens on the host side is broken and cannot be relied upon. Additionally, there is little reason to think it measurably optimizes the operation of finding a token for an account especially since it's only executed from the validator
  4. discovered [BUG] Eq/Ord for PermissionToken implementations are broken #3857

Linked issue

Closes #3853

Benefits

Checklist

  • I've read CONTRIBUTING.md
  • I've used the standard signed-off commit format (or will squash just before merging)
  • All applicable CI checks pass (or I promised to make them pass later)
  • (optional) I've written unit tests for the code changes
  • I replied to all comments after code review, marking all implemented changes with thumbs up

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Aug 31, 2023
@mversic mversic force-pushed the consistent_token_payload_serialization branch 5 times, most recently from 43a6a34 to bbed046 Compare August 31, 2023 12:31
appetrosyan
appetrosyan previously approved these changes Aug 31, 2023
client/tests/integration/permissions.rs Show resolved Hide resolved
data_model/src/isi.rs Show resolved Hide resolved
data_model/src/permission.rs Show resolved Hide resolved
data_model/src/permission.rs Show resolved Hide resolved
default_validator/src/lib.rs Show resolved Hide resolved
default_validator/src/lib.rs Show resolved Hide resolved
@mversic mversic force-pushed the consistent_token_payload_serialization branch 3 times, most recently from 5e17907 to 842e035 Compare August 31, 2023 13:55
@coveralls
Copy link

coveralls commented Aug 31, 2023

Pull Request Test Coverage Report for Build 6038411647

  • 33 of 101 (32.67%) changed or added relevant lines in 4 files are covered.
  • 5538 unchanged lines in 89 files lost coverage.
  • Overall coverage decreased (-1.8%) to 57.664%

Changes Missing Coverage Covered Lines Changed/Added Lines %
data_model/src/visit.rs 0 5 0.0%
data_model/derive/src/lib.rs 33 39 84.62%
data_model/src/permission.rs 0 11 0.0%
data_model/src/isi.rs 0 46 0.0%
Files with Coverage Reduction New Missed Lines %
cli/src/main.rs 1 0.0%
cli/src/torii/pagination.rs 1 98.9%
config/base/src/runtime_upgrades.rs 1 51.72%
config/src/torii.rs 1 96.67%
core/src/smartcontracts/isi/block.rs 1 87.5%
crypto/src/merkle.rs 1 96.23%
data_model/derive/src/partially_tagged.rs 1 99.67%
data_model/src/account.rs 1 52.21%
data_model/src/domain.rs 1 47.37%
logger/src/lib.rs 1 95.15%
Totals Coverage Status
Change from base Build 5423219773: -1.8%
Covered Lines: 20604
Relevant Lines: 35731

💛 - Coveralls

@mversic mversic force-pushed the consistent_token_payload_serialization branch from 842e035 to b4d379b Compare August 31, 2023 14:05
Copy link
Contributor

@0x009922 0x009922 left a comment

Choose a reason for hiding this comment

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

I am not familiar with the changes well. On the shallow look, LGTM. Since this PR is blocking us from rc19 release, I am approving this with fingers crossed.

@mversic mversic merged commit 8d38945 into hyperledger-iroha:iroha2-dev Sep 1, 2023
11 checks passed
@mversic mversic deleted the consistent_token_payload_serialization branch September 1, 2023 10:11
mversic added a commit that referenced this pull request Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants