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: e2e working with aggregation 2/2 of message_id and merkle_root #2861

Merged
merged 24 commits into from
Nov 7, 2023

Conversation

nambrot
Copy link
Contributor

@nambrot nambrot commented Oct 26, 2023

Description

  • Removed the double mapping logic from the the merkle_tree_builder.get_proof() for fixing the following issue:

get_proof was expecting a nonce which it then uses to retrieve the leaf_index i.e. nonce -> message_id -> leaf_index and then prove_against_previous but in our case, we already got the leaf_index in merkle_tree_multisig.rs so what we end up doing is trying to look up the leaf_index twice and either not finding the message_id or proving wrong leaf_index which both causes the "cannot fetch metadata" for the merkle tree builder

Drive-by changes

  • additional logging for aggregation to indicate which ism (moduleType, address) we are missing
  • trace -> debug logging for sign_and_submit_checkpoint

Related issues

Backward compatibility

Yes

Testing

e2e

@nambrot nambrot requested a review from tkporter as a code owner October 26, 2023 18:55
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #2861 (c4431e5) into v3 (09550c3) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##               v3    #2861   +/-   ##
=======================================
  Coverage   66.96%   66.96%           
=======================================
  Files          99       99           
  Lines        1017     1017           
  Branches      106      106           
=======================================
  Hits          681      681           
  Misses        296      296           
  Partials       40       40           
Components Coverage Δ
core 50.00% <ø> (ø)
hooks 71.42% <ø> (ø)
isms 71.22% <ø> (ø)
token 58.41% <ø> (ø)
middlewares 81.46% <ø> (ø)

@aroralanuk aroralanuk changed the title Nambrot/repro only merkle tree fix: e2e working with aggregation 2/2 of message_id and merkle_root Nov 3, 2023
Copy link
Member

@yorhodes yorhodes left a comment

Choose a reason for hiding this comment

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

not sure what the unrelated changes are but probably @tkporter or @daniel-savu should approve first if you want to merge those

rust/agents/relayer/src/merkle_tree/builder.rs Outdated Show resolved Hide resolved
rust/agents/relayer/src/msg/metadata/aggregation.rs Outdated Show resolved Hide resolved
rust/agents/relayer/src/msg/metadata/aggregation.rs Outdated Show resolved Hide resolved
rust/agents/relayer/src/merkle_tree/builder.rs Outdated Show resolved Hide resolved
rust/agents/relayer/src/merkle_tree/builder.rs Outdated Show resolved Hide resolved
rust/agents/relayer/src/msg/metadata/aggregation.rs Outdated Show resolved Hide resolved
rust/agents/relayer/src/msg/metadata/aggregation.rs Outdated Show resolved Hide resolved
rust/agents/relayer/src/msg/metadata/aggregation.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@tkporter tkporter left a comment

Choose a reason for hiding this comment

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

lmk if you wanna chat about my suggestions - motivation is to remove all unwraps and to avoid the extra work just for logging purposes

rust/agents/relayer/src/msg/metadata/aggregation.rs Outdated Show resolved Hide resolved
rust/agents/relayer/src/msg/metadata/aggregation.rs Outdated Show resolved Hide resolved
rust/agents/relayer/src/msg/metadata/aggregation.rs Outdated Show resolved Hide resolved
@aroralanuk aroralanuk requested a review from tkporter November 7, 2023 02:45
@aroralanuk aroralanuk enabled auto-merge (squash) November 7, 2023 19:59
@aroralanuk aroralanuk mentioned this pull request Nov 7, 2023
@aroralanuk aroralanuk merged commit e4eed2a into v3 Nov 7, 2023
@aroralanuk aroralanuk deleted the nambrot/repro-only-merkle-tree branch November 7, 2023 20:21
@yorhodes yorhodes mentioned this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants