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

[Solana] Optional merkle_root on commit accepted event #639

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

PabloMansanet
Copy link
Contributor

No description provided.

@PabloMansanet PabloMansanet requested a review from a team as a code owner February 20, 2025 15:56
@PabloMansanet PabloMansanet changed the title [Solana] Optinal merkle_root on commit accepted event [Solana] Optional merkle_root on commit accepted event Feb 20, 2025
Copy link

Metric optional_merkle_root_event main
Coverage 74.1% 74.1%

// Will be zero'd (MerkleRoot::Default() in case the commit report included no
// merkle root. This is to circumvent a Go deserialization issue.)
pub merkle_root: MerkleRoot,
pub merkle_root: Option<MerkleRoot>,
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to communicate this change with the following teams:

  • Chain Reader
  • Log Poller
  • Observability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's chat about it offline, I'm not fully aware yet of the channels/people through which to communicate changes like this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've requested this change to fix decoding the report in CR/offchain so LGTM 👍🏻

}

// This event uses bespoke parsing, as it contains an `Optional` field which cannot be unmarshalled through `bin.UnmarshalBorsh`
func ParseEventCommitReportAccepted(logs []string, event string, obj *EventCommitReportAccepted) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment here on why we need to do this differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the comment in line 85 not sufficient? It was my intention to explain it with that comment 🙂 (plus the one in ParseEvent which describes why it won't work with events that have an Option).

If you are asking about the root cause, something is just not right in the borsh decoder implementation of the gagliardetto/bin library, and it doesn't particularly seem worth the time to investigate it further.

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