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

core/types: transaction conditional KnownAccounts fix && HexOrDecimal deser #414

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

hamdiallam
Copy link
Contributor

@hamdiallam hamdiallam commented Oct 25, 2024

KnownAccounts should not use a pointer receiver for MarshalJSON. We can also leverage the math.HexOrDecimal to be more lenient when deserializing the conditional number values

@hamdiallam hamdiallam requested a review from a team as a code owner October 25, 2024 16:41
@hamdiallam hamdiallam force-pushed the transactionCondition.hexordecimal branch from 6d38391 to 33c3c9f Compare October 28, 2024 18:50
@hamdiallam hamdiallam changed the title core/types: transaction conditional HexOrDecimal deser core/types: transaction conditional KnownAccounts fix && HexOrDecimal deser Oct 28, 2024
@hamdiallam hamdiallam force-pushed the transactionCondition.hexordecimal branch from 33c3c9f to 87c5698 Compare October 28, 2024 18:53
Copy link
Contributor

@tynes tynes left a comment

Choose a reason for hiding this comment

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

Please fix and squash into a single commit with a clean commit message that follows the style of geth before merge

@hamdiallam hamdiallam force-pushed the transactionCondition.hexordecimal branch from 87c5698 to 3c993d8 Compare October 29, 2024 13:51
@hamdiallam
Copy link
Contributor Author

@tynes updated! Can you lmk what you mean by geth style commit message? I think the title reflects what I see in the geth PRs, package: description, and there doesn't seem to be a format for the description

@tynes
Copy link
Contributor

tynes commented Oct 29, 2024

@tynes updated! Can you lmk what you mean by geth style commit message? I think the title reflects what I see in the geth PRs, package: description, and there doesn't seem to be a format for the description

The title of the PR approximates a geth commit message, like you said package name: description. I notice that your commit message now doesn't follow that but the PR title does instead? We try to keep the git history of op-geth very clean so that its possible to easily view the diff over time.

I also think its a bad idea to manually modify autogenerated files, I notice that the allow unused fields diff is lost in this PR, I missed that in review previously and wouldn't have approved that if I noticed it at the time

@hamdiallam hamdiallam force-pushed the transactionCondition.hexordecimal branch from 3c993d8 to 6d4a826 Compare October 29, 2024 15:25
@hamdiallam
Copy link
Contributor Author

@tynes updated! Can you lmk what you mean by geth style commit message? I think the title reflects what I see in the geth PRs, package: description, and there doesn't seem to be a format for the description

The title of the PR approximates a geth commit message, like you said package name: description. I notice that your commit message now doesn't follow that but the PR title does instead? We try to keep the git history of op-geth very clean so that its possible to easily view the diff over time.

I also think its a bad idea to manually modify autogenerated files, I notice that the allow unused fields diff is lost in this PR, I missed that in review previously and wouldn't have approved that if I noticed it at the time

Ahh wait I think I misunderstood. Didn't realize the commit message wasn't just the PR title. Renamed the commit title to match

And noted on the generated code. The generator doesn't seem to have an option for unknown fields but that's not super important anyways

@hamdiallam hamdiallam merged commit a52bc72 into optimism Oct 29, 2024
9 of 10 checks passed
@hamdiallam hamdiallam deleted the transactionCondition.hexordecimal branch October 29, 2024 15:39
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