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(plugin-evm): base LiFi class inheritance #991

Closed
wants to merge 3 commits into from

Conversation

jdubpark
Copy link

Relates to:

None

Risks

Low, simple refactoring + removal of an unused parameter (mismatching type).

Background

What does this PR do?

  • Refactor the SwapAction and BridgeAction, both of which use the LiFi SDK, to inherit BaseLifiAction, which de-duplicates the constructor for the two actions.
  • Remove this.config from executeRoute in Swap & Bridge actions, as the type (SDKOptions) and values of this.config do not match and are not intended to be used in executeRoute, which expects hooks via the type ExecutionOptions.
  • Reorder import ordering
  • Add IDE folders in .gitignore

What kind of change is this?

  • Bug fix: remove unused/misused parameter (found due to strict type checking, SDKConfig =/= ExecutionOptions)
  • Improvements: refactoring + gitignore

Testing

Where should a reviewer start?

Does not change the existing logic of the plugin-evm.

Detailed testing steps

None

@jdubpark jdubpark changed the title feat(plugin-evm): base LiFi class inheritance fix(plugin-evm): base LiFi class inheritance Dec 11, 2024
Copy link
Collaborator

@monilpat monilpat left a comment

Choose a reason for hiding this comment

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

Thanks for working on this lgtm - outside of resolving merge conflicts and adding a screengrab or any confirmation that this does not introduce regressions

@nicky-ru
Copy link
Contributor

adding related PR here: #864

@odilitime
Copy link
Collaborator

While the refactor is nice I don't think we want to always utilize LiFi, as these are the potential downsides:

  • Tight Coupling to LiFi SDK
  • Reduced Readability
  • Technical Debt from Future SDK Changes

@odilitime odilitime closed this Dec 28, 2024
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.

4 participants