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

Update StagingStatusUpdated.code event value to SHA3_256 hash #38

Merged
merged 6 commits into from
Jul 26, 2024

Conversation

sisyphusSmiling
Copy link
Contributor

@sisyphusSmiling sisyphusSmiling commented Jul 26, 2024

Closes: #37


For contributor use:

  • Targeted PR against main branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the standards mentioned here.
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@sisyphusSmiling sisyphusSmiling self-assigned this Jul 26, 2024
@sisyphusSmiling sisyphusSmiling added the bug Something isn't working label Jul 26, 2024
@sisyphusSmiling sisyphusSmiling marked this pull request as ready for review July 26, 2024 18:21
@sisyphusSmiling sisyphusSmiling requested a review from a team as a code owner July 26, 2024 18:21
@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.81%. Comparing base (5f45a7f) to head (f165996).

Files Patch % Lines
contracts/MigrationContractStaging.cdc 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #38      +/-   ##
==========================================
- Coverage   83.91%   83.81%   -0.11%     
==========================================
  Files           6        6              
  Lines         342      346       +4     
==========================================
+ Hits          287      290       +3     
- Misses         55       56       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

Maybe change the parameter code: String of the event StagingStatusUpdated to codeHash: [UInt8; 32] to make the event less stringly typed and reduce the overhead of hex-encoding

@sisyphusSmiling
Copy link
Contributor Author

Maybe change the parameter code: String of the event StagingStatusUpdated to codeHash: [UInt8; 32] to make the event less stringly typed and reduce the overhead of hex-encoding

Makes sense. cc-ing @bluesign in case his tooling depends on events at all. I think not, but tagging just in case

@sisyphusSmiling sisyphusSmiling merged commit 8a179be into main Jul 26, 2024
1 check passed
@sisyphusSmiling sisyphusSmiling deleted the fix-event-limit branch July 26, 2024 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[BUG] Running into event limits on staging for long contracts
4 participants