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

commit plugin ocr3 - Code cleanup and minor fixes #864

Merged
merged 1 commit into from
May 16, 2024

Conversation

dimkouv
Copy link
Member

@dimkouv dimkouv commented May 16, 2024

  • Convert method to function
  • Rename var to not shadow function
  • Minor refactoring
  • Include token prices in outcome
  • ...

@dimkouv dimkouv requested a review from a team as a code owner May 16, 2024 09:27
@dimkouv dimkouv requested a review from makramkd May 16, 2024 09:27
Copy link
Contributor

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset.

@@ -186,6 +186,11 @@ func (p *Plugin) ObservationQuorum(_ ocr3types.OutcomeContext, _ types.Query) (o
// - Merkle Roots: One merkle tree root per source chain. The leaves of the tree are the IDs of the observed messages.
// The merkle root data type contains information about the chain and the sequence numbers range.
func (p *Plugin) Outcome(_ ocr3types.OutcomeContext, _ types.Query, aos []types.AttributedObservation) (ocr3types.Outcome, error) {
fChainDest, ok := p.cfg.FChain[p.cfg.DestChain]
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note but this is something good to check when creating the plugin. Can put a TODO/ticket for the future


/*
Once token/gas prices are implemented, we would want to probably check if outc.MerkleRoots is empty or not
todo: Once token/gas prices are implemented, we would want to probably check if outc.MerkleRoots is empty or not
Copy link
Contributor

Choose a reason for hiding this comment

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

Good to create a ticket for this as well, I think the timer/timestamp for this would have to be in the outcome right, so that the committee can agree on it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Compare prevOutcome.Timestamp to current timestamp, if > 30 min or whatever (configurable) then include prices in the outcome?

@dimkouv dimkouv merged commit 33f6444 into ccip-develop May 16, 2024
77 of 78 checks passed
@dimkouv dimkouv deleted the dk/small-fixes branch May 16, 2024 11:16
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.

None yet

2 participants