Skip to content
This repository was archived by the owner on Apr 11, 2023. It is now read-only.

feat(swapper): remove deprecated getSwapper method #853

Closed
wants to merge 2 commits into from

Conversation

0xApotheosis
Copy link
Member

@0xApotheosis 0xApotheosis commented Jul 2, 2022

getSwapper is deprecated, and is now only used in tests.

This PR removes it and uses Map.prototype.get() in the tests instead.

@0xApotheosis 0xApotheosis requested review from a team and kaladinlight as code owners July 2, 2022 03:42
gomesalexandre
gomesalexandre previously approved these changes Jul 2, 2022
Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

LGTM, all references to manager.getSwapper() are removed in tests and replaced by Map.prototype.get() 🍬

@0xdef1cafe 0xdef1cafe linked an issue Jul 5, 2022 that may be closed by this pull request
'[validateSwapper] - invalid swapper instance'
)
expect(swapper.swappers.get(SwapperType.Thorchain)).toBeInstanceOf(ThorchainSwapper)
expect(swapper.swappers.get(SwapperType.Zrx)).toBeInstanceOf(ZrxSwapper)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(swapper.swappers.get(SwapperType.Zrx)).toBeInstanceOf(ZrxSwapper)

@kaladinlight
Copy link
Contributor

This feels a little weird to me having get be the only method interacting directly with the map versus add/remove/etc using the helper functions. To me it seems like we either want to just interact with the map directly (a-la chainAdapterManager), or if there is extra necessary logic as part of the swapper manager (ie. getBest) that we don't want to duplicate in the client, then we can just have the swapper manager class and interact with the provided methods imo.

@0xdef1cafe
Copy link
Collaborator

as discussed - let's talk to the manager through it's interfaces it exposes, rather than the raw map of swappers

@0xdef1cafe 0xdef1cafe closed this Jul 6, 2022
@0xdef1cafe
Copy link
Collaborator

@0xApotheosis plz undeprecate this (lol) as to not cause future confusion

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants