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

runtime/consensus: Add support for querying round roots #1564

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented Nov 21, 2023

No description provided.

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (513c2c4) 60.51% compared to head (4628dbd) 60.44%.

Files Patch % Lines
runtime-sdk/src/modules/consensus/mod.rs 7.69% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1564      +/-   ##
==========================================
- Coverage   60.51%   60.44%   -0.07%     
==========================================
  Files         139      139              
  Lines        9942     9955      +13     
==========================================
+ Hits         6016     6017       +1     
- Misses       3884     3896      +12     
  Partials       42       42              

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

@ptrus ptrus force-pushed the ptrus/feature/round-roots branch 2 times, most recently from a6b0094 to 08e8d4e Compare November 21, 2023 16:02
@ptrus ptrus marked this pull request as ready for review November 21, 2023 16:03
#[repr(u8)]
pub enum RootKind {
#[default]
Invalid = 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this?

Copy link
Member Author

@ptrus ptrus Nov 21, 2023

Choose a reason for hiding this comment

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

This follows the same pattern as it is already used by the other internal call in the code (consensus.TakeReceipt). Not actually sure, but I guessed we have the invalid as the default variant as an additional safeguard, since the cbor encoding can be a bit messy in soldity. Or my other guess would be it has to do with gas costs in evm (to ensure it's the same regardless of the kind).

@kostko please confirm what is the reasoning and if it makes sense to have it here as well.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this would be useful, we could just drop the invalid variant and start the valid ones with 1 (as it is now).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I removed the Invalid variant here.

@ptrus ptrus force-pushed the ptrus/feature/round-roots branch 2 times, most recently from 4dd442c to a5bbbc3 Compare November 23, 2023 15:54
@ptrus ptrus requested a review from peternose November 23, 2023 15:55
Copy link
Member

Choose a reason for hiding this comment

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

Do not commit this, consider adding to gitignore.

}

/*
// Round Roots not exposed in the go roothash client at the moment :(
Copy link
Member

Choose a reason for hiding this comment

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

Feel free to add the query.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do but don't want to block this PR until the next oasis-core release.

#[repr(u8)]
pub enum RootKind {
#[default]
Invalid = 0,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why this would be useful, we could just drop the invalid variant and start the valid ones with 1 (as it is now).

@ptrus ptrus force-pushed the ptrus/feature/round-roots branch 2 times, most recently from f74737f to d153ab0 Compare November 23, 2023 16:26
@ptrus ptrus force-pushed the ptrus/feature/round-roots branch from d153ab0 to 4628dbd Compare November 23, 2023 16:32
@ptrus ptrus requested a review from kostko November 23, 2023 16:37
@ptrus ptrus merged commit aa11ed5 into main Nov 23, 2023
26 of 28 checks passed
@ptrus ptrus deleted the ptrus/feature/round-roots branch November 23, 2023 17:01
github-actions bot added a commit that referenced this pull request Nov 23, 2023
…trus/feature/round-roots

runtime/consensus: Add support for querying round roots aa11ed5
github-actions bot added a commit that referenced this pull request Nov 23, 2023
…/ptrus/feature/round-roots

runtime/consensus: Add support for querying round roots aa11ed5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants