refactor: replace wrapped with address #2364
Conversation
🦋 Changeset detectedLatest commit: 1e9d945 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| this.mutateBalances = Boolean(mutateBalances); | ||
|
|
||
| //call to super ensures this array access is safe | ||
| if (tokens[0].isUnderlyingEqual(swapAmount.token)) { |
There was a problem hiding this comment.
@brunoguerios I am slowly progressing through replacing references to Token with BaseToken (working name). Commenting here as I had to refactor to use functionality of the BaseToken here now instead of Token.
There are other opportunities for me to replace Token with BaseToken (think TokenAmount). However, if I do these changes in the sdk. (TokenAmount can either accept Token or BaseToken in the constructor I am getting alot of downstream build errors in the sdk.
So, the question here is:
- Would a "perfect" refactor also need to change how
TokenAmountis used?
There was a problem hiding this comment.
Can you please elaborate on which changes would be required to TokenAmount?
Things such as replacing isUnderlyingEqual with isSameAddress make sense 👍
Ps: regarding variable naming, my suggestion would be to keep Token as the base token (without wrapped) and have a NativeToken (with wrapped). Which should result in less downstream changes compared to renaming Token with BaseToken.
Ps2: I'd expect TokenAmount could accept only Token (without wrapped) - if that's not the case, then it's important to evaluate if this refactor won't increase complexity in any way. The overall idea is to aim for more simplicity and type safety.
There was a problem hiding this comment.
I have almost fully replaced the Token with BaseToken (which does not include wrapped). This led to quite a big dif.
|
@brunoguerios @mkflow27 can this be merged? |
Hey @franzns This required balancer/b-sdk#725 to be merged. We'll take a look |
|
I released b-sdk with the required updates, but since it's a major version bump I need to make sure things are working as expected over here. It should likely be ready for review tomorrow |
Closes #830
Working off of this feature branch. balancer/b-sdk#725