Skip to content

refactor: address nits - #1989

Merged
DariusParvin merged 1 commit into
masterfrom
BG-42470-clean-up
Feb 17, 2022
Merged

refactor: address nits#1989
DariusParvin merged 1 commit into
masterfrom
BG-42470-clean-up

Conversation

@DariusParvin

@DariusParvin DariusParvin commented Feb 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

addressing nits from #1944 and #2004

  • remove external signing routes and comments for v1 wallet
  • remove unecessary prototpe set in ExternalSignerConfigError
  • add more descriptive debug msg for retries to external signer
  • remove unnecessary catch block

Ticket: BG-42470

@codecov

codecov Bot commented Feb 10, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1989 (8d3f77b) into master (fa84b50) will increase coverage by 53.61%.
The diff coverage is 100.00%.

❗ Current head 8d3f77b differs from pull request most recent head 17ce690. Consider uploading reports for the commit 17ce690 to get more accurate results

@@             Coverage Diff             @@
##           master    #1989       +/-   ##
===========================================
+ Coverage        0   53.61%   +53.61%     
===========================================
  Files           0        5        +5     
  Lines           0      526      +526     
  Branches        0       81       +81     
===========================================
+ Hits            0      282      +282     
- Misses          0      240      +240     
- Partials        0        4        +4
Flag Coverage Δ
unit 53.61% <100.00%> (?)

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

Impacted Files Coverage Δ
modules/express/src/clientRoutes.ts 32.04% <ø> (ø)
modules/express/src/errors.ts 86.66% <ø> (ø)
modules/express/src/expressApp.ts 89.68% <100.00%> (ø)
modules/express/src/config.ts 100.00% <0.00%> (ø)
modules/express/src/args.ts 100.00% <0.00%> (ø)

@DariusParvin
DariusParvin marked this pull request as ready for review February 10, 2022 22:36
@DariusParvin
DariusParvin force-pushed the BG-42470-clean-up branch 2 times, most recently from 50ec14e to 1430a88 Compare February 17, 2022 20:13
tylerlevine
tylerlevine suggested changes Feb 17, 2022
View reviewed changes

@tylerlevine tylerlevine left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good clean up - could you add a fix for the nit I mentioned in #2004 as well?

Comment thread modules/express/src/clientRoutes.ts Outdated

@tylerlevine tylerlevine Feb 17, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we not supporting external signer mode for these functions where the comments are getting removed?

@DariusParvin DariusParvin Feb 17, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah I believe that's the idea since they are all v1 wallet functions. Currently if signer mode is enabled, the v1 wallet functions would just operate the same as before.

@tylerlevine tylerlevine Feb 17, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, right - sounds good then

Comment thread modules/express/src/errors.ts Outdated

@tylerlevine tylerlevine Feb 17, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm curious why we're getting rid of this error in favor of throwing the top level Error type instead?

@DariusParvin DariusParvin Feb 17, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah okay I realize now that I misunderstood this original comment. #1944 (comment)

I believe you meant that just Object.setPrototypeOf(this, ExternalSignerConfigError.prototype); was unnecessary. I'll edit this commit.

Darius Parvin
refactor: address nits
17ce690 
remove external signing routes for v1 wallet
remove outdated comments
remove unecessary prototpe set in ExternalSignerConfigError
add more descriptive debug msg

Ticket: BG-42470
@DariusParvin

DariusParvin commented Feb 17, 2022

Copy link
Copy Markdown
Contributor Author

rebased and addressed the comments including the nit from #2004

@DariusParvin DariusParvin mentioned this pull request Feb 17, 2022
Merged
tylerlevine
tylerlevine approved these changes Feb 17, 2022
View reviewed changes
Comment thread modules/express/src/clientRoutes.ts Outdated

@tylerlevine tylerlevine Feb 17, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah, right - sounds good then

@DariusParvin
DariusParvin merged commit 7f70459 into master Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@tylerlevine tylerlevine tylerlevine approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@DariusParvin @tylerlevine