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.

Sign up for GitHub

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

Jump to bottom

Rename DA oracle consts to be more descriptive #14743

Merged
merged 2 commits into from
Oct 16, 2024
Merged

Rename DA oracle consts to be more descriptive #14743

merged 2 commits into from
Oct 16, 2024

Conversation

ogtownsend
Copy link
Collaborator

@ogtownsend ogtownsend commented Oct 11, 2024 •
edited
Loading

  • Referencing the oracle types as toml.OPStack, toml.Arbitrum looked and felt a little odd (see feedback here)
  • Creates a new package daoracle and moves the DA oracle config over so we can now reference them as daoracle.OPStack, etc
  • Renames these DA oracle consts to be more descriptive

@ogtownsend ogtownsend requested a review from matYang October 11, 2024 22:28
@ogtownsend ogtownsend marked this pull request as ready for review October 11, 2024 22:28
@ogtownsend ogtownsend requested review from a team as code owners October 11, 2024 22:28
@ogtownsend ogtownsend requested a review from a team as a code owner October 11, 2024 22:33
@ogtownsend ogtownsend requested a review from jmank88 October 11, 2024 22:33
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 11, 2024 •
edited
Loading

Below is an analysis created by an LLM. Be mindful of hallucinations and verify accuracy.

WF: CI Core#f0c928b

1. HTTP 503 Service Temporarily Unavailable error during push request:[Flakey Test Detection]

Source of Error: 
Flakey Test Detection	Re-run tests	2024-10-15T23:18:16.0201636Z 2024/10/15 23:18:16 Error re-running flakey tests: push request failed: status=503, body=<html>
Flakey Test Detection	Re-run tests	2024-10-15T23:18:16.0204057Z <head><title>503 Service Temporarily Unavailable</title></head>
Flakey Test Detection	Re-run tests	2024-10-15T23:18:16.0204977Z <body>
Flakey Test Detection	Re-run tests	2024-10-15T23:18:16.0205839Z <center><h1>503 Service Temporarily Unavailable</h1></center>
Flakey Test Detection	Re-run tests	2024-10-15T23:18:16.0206750Z <hr><center>nginx</center>
Flakey Test Detection	Re-run tests	2024-10-15T23:18:16.0208695Z </body>
Flakey Test Detection	Re-run tests	2024-10-15T23:18:16.0209361Z </html>

Why: The error occurred because the server handling the push request was temporarily unable to handle the request, possibly due to maintenance or overload.

Suggested fix: Retry the operation after some time or investigate server status and load to ensure it can handle incoming requests.

@ogtownsend ogtownsend requested review from patrickhuie19 and amit-momin and removed request for jmank88, rstout, makramkd, dimkouv, mateusz-sekara and 0xnogo October 11, 2024 22:39
@ogtownsend ogtownsend force-pushed the chore/move-da-oracle-config-consts branch 4 times, most recently from 4528a9b to 3d38239 Compare October 15, 2024 17:24
@ogtownsend ogtownsend force-pushed the chore/move-da-oracle-config-consts branch from 3d38239 to f0c928b Compare October 15, 2024 18:01
amit-momin
amit-momin previously approved these changes Oct 15, 2024
View reviewed changes
winder
winder previously approved these changes Oct 15, 2024
View reviewed changes
patrickhuie19
patrickhuie19 previously approved these changes Oct 16, 2024
View reviewed changes
jmank88
jmank88 requested changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@jmank88 jmank88 left a comment

Choose a reason for hiding this comment

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

What is the purpose of creating an entirely new package? This needs to be justified, and then if it makes sense we need to find a non-redundant name.

If you are finding it hard to think of a non-redundant name, consider that is a sign that you have isolated too little in a package on it's own.

@ogtownsend
Copy link
Collaborator Author

ogtownsend commented Oct 16, 2024 •
edited
Loading

What is the purpose of creating an entirely new package? This needs to be justified, and then if it makes sense we need to find a non-redundant name.

If you are finding it hard to think of a non-redundant name, consider that is a sign that you have isolated too little in a package on it's own.

@jmank88 This PR was motivated by @matYang's comment here: #14710 (comment)

The update to oracletype was based on a preference and suggestion from @amit-momin to make it more similar to chaintype (in a slack thread).

I agree that referencing the oracle type with the toml package toml.<oracle_type> seems pretty odd. So maybe the better approach that avoids creating a new package but is still explicit is to append the oracle type with DAOracle_..., so instead it would be toml.DAOracle_OPStack, toml.DAOracle_Arbitrum, etc...

@jmank88
Copy link
Contributor

jmank88 commented Oct 16, 2024

I agree that referencing the oracle type with the toml package toml.<oracle_type> seems pretty odd. So maybe the better approach that avoids creating a new package but is still explicit is to append the oracle type with DAOracle_..., so instead it would be toml.DAOracle_OPStack, toml.DAOracle_Arbitrum, etc...

Idiomatic Go names do not use underscores, so toml.DAOracleOPStack, toml.DAOracleArbitrum but what is DA? If just "oracle type" was clear before, then why is a DA qualifier necessary? Why not just toml.OracleTypeOPStack, toml.OracleTypeArbitrum?

@matYang
Copy link
Contributor

matYang commented Oct 16, 2024

toml.DAOracleOPStack looks ok; we want DA as a prefix to signal this is specifically for Data Availability, ie. Data Availability Oracle, Oracle by itself is overly generic.

@ogtownsend ogtownsend dismissed stale reviews from patrickhuie19, winder, and amit-momin via 4a8cbf2 October 16, 2024 16:24
@ogtownsend ogtownsend force-pushed the chore/move-da-oracle-config-consts branch from f0c928b to 4a8cbf2 Compare October 16, 2024 16:24
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 16, 2024 •
edited
Loading

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@ogtownsend ogtownsend changed the title Move daoracle config into own package Rename DA oracle consts to be more descriptive Oct 16, 2024
@ogtownsend ogtownsend force-pushed the chore/move-da-oracle-config-consts branch from 4a8cbf2 to 38910fe Compare October 16, 2024 20:46
ogtownsend
ogtownsend commented Oct 16, 2024
View reviewed changes
core/chains/evm/config/toml/config.go
OracleAddress *types.EIP55Address
CustomGasPriceCalldata string
}

type OracleType string
type DAOracleType string
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @jmank88 I also updated this to DAOracleType to be more descriptive since it will continue to live in the toml package, and we won't be violating: https://go.dev/doc/effective_go#names

@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

@ogtownsend ogtownsend added this pull request to the merge queue Oct 16, 2024
Merged via the queue into develop with commit d5dfe4a Oct 16, 2024
147 checks passed
@ogtownsend ogtownsend deleted the chore/move-da-oracle-config-consts branch October 16, 2024 22:26
This was referenced Oct 16, 2024
Draft
Draft
Draft
@github-actions github-actions bot mentioned this pull request Oct 24, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amit-momin amit-momin amit-momin approved these changes

@jmank88 jmank88 jmank88 approved these changes

@winder winder winder approved these changes

@patrickhuie19 patrickhuie19 patrickhuie19 left review comments

@matYang matYang Awaiting requested review from matYang

@asoliman92 asoliman92 Awaiting requested review from asoliman92 asoliman92 is a code owner automatically assigned from smartcontractkit/ccip-offchain

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ogtownsend @jmank88 @matYang @winder @patrickhuie19 @amit-momin