-
Notifications
You must be signed in to change notification settings - Fork 23
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
imp: add support for electra #375
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #375 +/- ##
=======================================
Coverage 99.86% 99.86%
=======================================
Files 14 14
Lines 731 731
=======================================
Hits 730 730
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. Did my initial pass. Let's try to address as much of these as we can before merging.
@@ -88,6 +90,7 @@ func (s *IbcEurekaTestSuite) SetupSuite(ctx context.Context, proofType operator. | |||
eth, simd := s.EthChain, s.CosmosChains[0] | |||
|
|||
var prover string | |||
shouldGenerateRustFixtures := false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable seems useless
@@ -326,11 +332,13 @@ where | |||
}, | |||
account_update, | |||
consensus_update: update.clone(), | |||
}); | |||
}; | |||
headers.push(header.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: You can add the debug statement before this push and remove the clone if you want since pushing cannot fail anyway...
impl IICS26RouterMsgs::Payload { | ||
/// Returns the commitment path for the payload. | ||
#[must_use] | ||
pub fn commitment_hash(&self) -> Vec<u8> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need not be public
pub fn commitment_hash(&self) -> Vec<u8> { | |
fn commitment_hash(&self) -> Vec<u8> { |
@@ -29,6 +30,37 @@ impl IICS26RouterMsgs::Packet { | |||
path | |||
} | |||
|
|||
/// Returns the packet commitment | |||
#[must_use] | |||
pub fn commit_packet(&self) -> Vec<u8> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be renamed imo.
pub fn commit_packet(&self) -> Vec<u8> { | |
pub fn packet_commitment(&self) -> Vec<u8> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda of agree, but I was using the naming from ibc-go to just be consistent. I'm OK with either honestly
let mut app_bytes = Vec::new(); | ||
for payload in &self.payloads { | ||
app_bytes.extend_from_slice(&payload.commitment_hash()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: You should be able to do this via iterator methods on self.payloads
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -27,11 +27,13 @@ pub enum EthereumIBCError { | |||
#[error("insufficient number of sync committee participants ({0})")] | |||
InsufficientSyncCommitteeParticipants(u64), | |||
|
|||
#[error("update header contains deneb specific information")] | |||
MustBeDeneb, | |||
#[error("unsupported fork version, we only support electra")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this should be MustBeElectra, or we will forget to update this string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, maybe that's better, will change to that!
/// `get_generalized_index(BeaconState, "next_sync_committee")` | ||
pub const NEXT_SYNC_COMMITTEE_INDEX: u64 = 55; | ||
pub const NEXT_SYNC_COMMITTEE_GINDEX_ELECTRA: u64 = 87; | ||
/// `get_generalized_index(BeaconBlockBody, "execution_payload")` | ||
pub const EXECUTION_PAYLOAD_INDEX: u64 = 25; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub const EXECUTION_PAYLOAD_INDEX: u64 = 25; | |
pub const EXECUTION_PAYLOAD_GINDEX: u64 = 25; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, will do!
Description
closes: #368
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
godoc
comments.Files changed
in the GitHub PR explorer.SonarCloud Report
in the comment section below once CI passes.