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

fix: override config values use viper #1502

Merged
merged 12 commits into from
Aug 28, 2024
Merged

Conversation

yiweichi
Copy link
Member

@yiweichi yiweichi commented Aug 26, 2024

Purpose or design rationale of this PR

This PR aims to solve the bug when trying to override private key fields like gas_oracle_sender_private_key , commit_sender_private_key.
For now, the way we override config fields is based on the tag of JSON marshaller, but the real tag of those private keys are json:"-", so here I propose to handle it specifically as a private key when the tag is -.
Didn't use Viper because seems that will involve quite amount of code changes.

PR title

Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:

  • fix: A bug fix

Deployment tag versioning

Has tag in common/version.go been updated or have you added bump-version label to this PR?

  • Yes

Breaking change label

Does this PR have the breaking-change label?

  • No, this PR is not a breaking change

@yiweichi yiweichi requested review from dghelm, colinlyguo and Thegaram and removed request for dghelm August 26, 2024 20:23
@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2024

Codecov Report

Attention: Patch coverage is 56.62651% with 36 lines in your changes missing coverage. Please review.

Project coverage is 51.61%. Comparing base (a528103) to head (93f5379).

Files Patch % Lines
rollup/internal/controller/relayer/l2_relayer.go 47.22% 14 Missing and 5 partials ⚠️
rollup/internal/config/config.go 68.29% 8 Missing and 5 partials ⚠️
rollup/internal/controller/relayer/l1_relayer.go 33.33% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1502      +/-   ##
===========================================
- Coverage    51.71%   51.61%   -0.10%     
===========================================
  Files          156      155       -1     
  Lines        12763    12771       +8     
===========================================
- Hits          6600     6592       -8     
- Misses        5595     5606      +11     
- Partials       568      573       +5     
Flag Coverage Δ
rollup 55.37% <56.62%> (-0.29%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@georgehao
Copy link
Member

Suggestion: use viper #1492 to replace the current implementation

@colinlyguo colinlyguo self-requested a review August 27, 2024 03:57
@colinlyguo
Copy link
Member

Suggestion: use viper #1492 to replace the current implementation

looks like the change is significant. @yiweichi what's the major part of code change? e.g. the self-implementing marshaling/unmarshaling private key part?

colinlyguo
colinlyguo previously approved these changes Aug 27, 2024
@colinlyguo colinlyguo mentioned this pull request Aug 27, 2024
@yiweichi
Copy link
Member Author

Suggestion: use viper #1492 to replace the current implementation

looks like the change is significant. @yiweichi what's the major part of code change? e.g. the self-implementing marshaling/unmarshaling private key part?

Yes, the major part would be marshaling/unmarshaling private key part.

@colinlyguo
Copy link
Member

Suggestion: use viper #1492 to replace the current implementation

looks like the change is significant. @yiweichi what's the major part of code change? e.g. the self-implementing marshaling/unmarshaling private key part?

Yes, the major part would be marshaling/unmarshaling private key part.

added a pr to try it. still a bit messy though.

Thegaram
Thegaram previously approved these changes Aug 27, 2024
@colinlyguo colinlyguo dismissed stale reviews from Thegaram and themself via 7c6d46a August 27, 2024 12:36
@yiweichi yiweichi changed the title fix: override private key config value fix: override config values use viper Aug 27, 2024
@yiweichi yiweichi merged commit 2ee1c89 into develop Aug 28, 2024
5 checks passed
@yiweichi yiweichi deleted the fix-override-config-private-key branch August 28, 2024 10:53
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