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

[BCI-3862][chainlink-common] - Change DSL Block primitive to string instead of int #683

Closed
wants to merge 6 commits into from

Conversation

Farber98
Copy link
Contributor

@Farber98 Farber98 commented Aug 2, 2024

Task Description:

Change ChainReader block DSL uint64 type to string and update EVM implementation to reflect changes

This PR:

Changed the block proto field and fixed all the reference failures within chainlink-common

Ticket:

Unblocks:

@@ -151,7 +151,7 @@ message Comparator {
}

message Block{
uint64 block_number = 1;
Copy link
Contributor Author

@Farber98 Farber98 Aug 2, 2024

Choose a reason for hiding this comment

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

Should I instead deprecate the previous field and add a new one because of backward compatibility? Could try if something like this works:

message Block {
  uint64 block_number_deprecated = 1 [deprecated = true]; // rename prev field and mark as deprecated
  string block_number = 3; // new field with string type that has the old name
  ComparisonOperator operator = 2;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually that is the best practice, but I don't see anyone using block querying so as long as you update tests in core evm it is fine to make a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

You'll have to update the PGDSLParser here, and check if any of the LP tests need to be fixed after this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Farber98
Copy link
Contributor Author

Farber98 commented Aug 2, 2024

"update EVM implementation to reflect changes" means updating this file? I don't see we are directly referencing the changed field

@Farber98 Farber98 changed the title [BCI-3862] - Change DSL Block primitive to string instead of int [BCI-3862][chainlink-common] - Change DSL Block primitive to string instead of int Aug 5, 2024
@Farber98 Farber98 marked this pull request as ready for review August 6, 2024 12:12
@Farber98 Farber98 requested review from jmank88, krehermann and a team as code owners August 6, 2024 12:12
jmank88
jmank88 previously approved these changes Aug 6, 2024
ilija42
ilija42 previously approved these changes Aug 7, 2024
cedric-cordenier and others added 2 commits August 7, 2024 13:24
* [KS-408] Handle nils in Unwrap/UnwrapTo

* Address feedback
@Farber98 Farber98 dismissed stale reviews from ilija42 and jmank88 via 8b09fef August 7, 2024 17:10
@Farber98 Farber98 force-pushed the BCI-3862-change-dsl-block-primitive-to-string branch from 71c871b to 8b09fef Compare August 7, 2024 17:10
@Farber98 Farber98 requested review from a team and Atrax1 as code owners August 7, 2024 17:10
@Farber98 Farber98 mentioned this pull request Aug 7, 2024
@Farber98 Farber98 closed this Aug 8, 2024
@Farber98
Copy link
Contributor Author

Farber98 commented Aug 8, 2024

reopened here with signed commits #688

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.

5 participants