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

Feature: enforce dataHash to be a hash of data, minor test enhancements #499

Merged
merged 6 commits into from
Jan 26, 2023

Conversation

mmv08
Copy link
Member

@mmv08 mmv08 commented Jan 24, 2023

This PR:

  • Implements Enforce that dataHash is the hash of data #497
    • To test the issue properly, I had to create a new function buildContractSignature and adjust buildSignatureBytes to support contract signatures with the dynamic part to avoid creating contract signatures in a raw manner
  • Improves tests for mixing all signature types with smart contract signatures by using newly created/adjusted functions from above. The tests were in fact incomplete because they were missing smart contract signature type

@mmv08 mmv08 requested review from a team, rmeissner and Uxio0 and removed request for a team January 24, 2023 17:31
@mmv08 mmv08 force-pushed the feature/checknsignatures-datahash branch 3 times, most recently from cd1d73c to 6398688 Compare January 24, 2023 17:33
@mmv08 mmv08 changed the title Feature: enforce dataHash to be a hash of data Feature: enforce dataHash to be a hash of data, minor test enhancements Jan 24, 2023
@coveralls
Copy link

Pull Request Test Coverage Report for Build 3998833487

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.5%) to 97.04%

Totals Coverage Status
Change from base Build 3947530336: -1.5%
Covered Lines: 305
Relevant Lines: 313

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3998833487

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.009%) to 98.52%

Totals Coverage Status
Change from base Build 3947530336: 0.009%
Covered Lines: 312
Relevant Lines: 313

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jan 24, 2023

Pull Request Test Coverage Report for Build 4007838837

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.009%) to 98.52%

Totals Coverage Status
Change from base Build 3947530336: 0.009%
Covered Lines: 312
Relevant Lines: 313

💛 - Coveralls

@mmv08 mmv08 force-pushed the feature/checknsignatures-datahash branch from 6398688 to 981c8ff Compare January 24, 2023 17:41
@Uxio0
Copy link
Member

Uxio0 commented Jan 25, 2023

Could you add more information in the PR about the "test enhancements" and the reasoning? They don't see very clear to me. Also maybe some comments in buildSignatureBytes will make us easier to understand in the future what's going on

@mmv08 mmv08 force-pushed the feature/checknsignatures-datahash branch from 981c8ff to e36fe25 Compare January 25, 2023 15:16
@mmv08
Copy link
Member Author

mmv08 commented Jan 25, 2023

Could you add more information in the PR about the "test enhancements" and the reasoning? They don't see very clear to me. Also maybe some comments in buildSignatureBytes will make us easier to understand in the future what's going on

updated the code and description, could you please recheck?

@Uxio0
Copy link
Member

Uxio0 commented Jan 25, 2023

Thanks for updating it @mikhailxyz , it makes sense now. Then after merging this we should close or refactor #416

@mmv08 mmv08 mentioned this pull request Jan 25, 2023
@mmv08 mmv08 force-pushed the feature/checknsignatures-datahash branch from e36fe25 to 58a5abf Compare January 25, 2023 16:18
@mmv08 mmv08 merged commit 8823fa3 into main Jan 26, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jan 26, 2023
@rmeissner rmeissner deleted the feature/checknsignatures-datahash branch April 19, 2023 18:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants