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

Map swap-related transfers #1691

Merged
merged 5 commits into from
Jun 27, 2024
Merged

Map swap-related transfers #1691

merged 5 commits into from
Jun 27, 2024

Conversation

iamacook
Copy link
Member

Summary

Incoming/outgoing transfers also cover transferFrom transactions. Swaps execute such transfers to/from the GPv2Settlement contract. Currently the decoding of these transactions are "standard".

This maps the relative order information to swap-related transfers if the sender/recipient address is the GPv2Settlement contract.

Changes

The following are predominantly code migrations of existing code as transfer mapping was assumed to happen with the TransactionInfoMapper, whereas it should in the TransferInfoMapper:

  • Create new SwapTransferTransactionInfo
  • Create new SwapTransferHelper
  • Create new SwapTransferInfoMapper and include it in TransferInfoMapper
  • Adjust the relative entities
  • Add/update test coverage accordingly

@iamacook iamacook self-assigned this Jun 25, 2024
@coveralls
Copy link

coveralls commented Jun 25, 2024

Pull Request Test Coverage Report for Build 9662861262

Details

  • 52 of 85 (61.18%) changed or added relevant lines in 10 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.2%) to 48.919%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/routes/transactions/mappers/common/swap-order.mapper.ts 0 1 0.0%
src/routes/transactions/mappers/transfers/transfer-info.mapper.ts 6 11 54.55%
src/routes/transactions/mappers/transfers/swap-transfer-info.mapper.ts 9 15 60.0%
src/routes/transactions/swap-transfer-transaction-info.entity.ts 28 49 57.14%
Files with Coverage Reduction New Missed Lines %
src/routes/transactions/mappers/transfers/transfer-info.mapper.ts 1 19.57%
Totals Coverage Status
Change from base Build 9661258751: 0.2%
Covered Lines: 4154
Relevant Lines: 6858

💛 - Coveralls

Base automatically changed from swap-api-orders to main June 26, 2024 14:50
Comment on lines +52 to +69
if (this.isSwapsDecodingEnabled && this.isTwapsDecodingEnabled) {
// If the transaction is a swap-based transfer, we return it immediately
const swapTransfer =
await this.swapTransferInfoMapper.mapSwapTransferInfo({
sender,
recipient,
direction,
transferInfo,
chainId,
safeAddress: safe.address,
domainTransfer,
});

if (swapTransfer) {
return swapTransfer;
}
}

Copy link
Member

@hectorgomezv hectorgomezv Jun 26, 2024

Choose a reason for hiding this comment

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

This way (provided the FFs are enabled), there is always a try to map the transfer as a SwapTransferTransactionInfo.

Alternatively, we could expose an isSwapTransferInfo method so we can integrate the matching/mapping logic into getTransferByType. That way we could check if the transfer is actually a SwapTransferTransactionInfo and then call mapSwapTransferInfo inside getTransferByType as we do to map the rest of the transfer types. Wdyt?

In terms of performance, it wouldn't improve the situation much, as mapSwapTransferInfo already early returns null if the conditions don't match, but in terms of readability, I think it could make the class API a bit clearer.

@coveralls
Copy link

coveralls commented Jun 26, 2024

Pull Request Test Coverage Report for Build 9684668354

Details

  • 53 of 102 (51.96%) changed or added relevant lines in 11 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.08%) to 48.808%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/routes/transactions/mappers/common/swap-order.mapper.ts 0 1 0.0%
src/routes/transactions/mappers/transfers/transfer-info.mapper.ts 6 11 54.55%
src/routes/transactions/swap-transfer-transaction-info.entity.ts 28 49 57.14%
src/routes/transactions/mappers/transfers/swap-transfer-info.mapper.ts 9 31 29.03%
Files with Coverage Reduction New Missed Lines %
src/routes/transactions/mappers/transfers/transfer-info.mapper.ts 1 19.57%
Totals Coverage Status
Change from base Build 9681910794: 0.08%
Covered Lines: 4193
Relevant Lines: 6929

💛 - Coveralls

Comment on lines +109 to +110
.with('buyToken', token.address)
.with('executedBuyAmount', BigInt(domainTransfer.value))
Copy link
Member

Choose a reason for hiding this comment

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

In these tests, I've just adjusted the Order payloads to match the logic implemented in SwapTransferInfoMapper.findOrderByTransfer not to filter out the order, i.e.: executedBuyAmount needs to match the transfer value, and buyToken address needs to match the token address. I'm assuming this logic is correct. cc. @iamacook @compojoom

@hectorgomezv hectorgomezv marked this pull request as ready for review June 27, 2024 06:42
@hectorgomezv hectorgomezv requested a review from a team as a code owner June 27, 2024 06:42
@hectorgomezv hectorgomezv merged commit 4069341 into main Jun 27, 2024
16 checks passed
@hectorgomezv hectorgomezv deleted the map-swap-transfers branch June 27, 2024 06:43
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.

3 participants