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

feat: add optional call to ERC20Handler and NativeTokenHandler execute #266

Merged
merged 17 commits into from
Sep 20, 2024

Conversation

lastperson
Copy link
Contributor

@lastperson lastperson commented Aug 21, 2024

Description

This change introduces a backward compatible extension to ERC20Handler that allows including optional message to be called on the token recipient address.
In general it is expected that the recipient of such call would be an intermediate proxy (like the one currently developed for Across) which will unpack the message into an array of actions (similar to how LiFi executor does).

Related Issue Or Context

Closes: #264 #265

How Has This Been Tested? Testing details.

Tests are WIP.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation

Checklist:

  • I have commented my code, particularly in hard-to-understand areas.
  • I have ensured that all acceptance criteria (or expected behavior) from issue are met
  • I have updated the documentation locally and in chainbridge-docs.
  • I have added tests to cover my changes.
  • I have ensured that all the checks are passing and green, I've signed the CLA bot

@lastperson lastperson changed the title Add optional call to ERC20Handler execute feat: add optional call to ERC20Handler execute Aug 21, 2024
@lastperson lastperson changed the title feat: add optional call to ERC20Handler execute feat: add optional call to ERC20Handler and NativeTokenHandler execute Aug 27, 2024
contracts/handlers/DefaultMessageReceiver.sol Outdated Show resolved Hide resolved
contracts/handlers/DefaultMessageReceiver.sol Show resolved Hide resolved
contracts/handlers/ERC20Handler.sol Outdated Show resolved Hide resolved
contracts/handlers/NativeTokenHandler.sol Show resolved Hide resolved
contracts/handlers/NativeTokenHandler.sol Outdated Show resolved Hide resolved
saadahmsiddiqui added a commit to sygmaprotocol/sygma-sdk that referenced this pull request Sep 10, 2024
…act Call (#504)

## Implementation details
With the introduction of the new `ERC20Handler`, the SDK needs to be
updated to handle the enhanced functionality that supports both ERC20
token transfers and optional contract calls in a single transaction.
[New ERC20Handler
PR](sygmaprotocol/sygma-solidity#266)

- Modify the SDK to work with the updated `ERC20Handler` by allowing it
to handle additional data that encodes contract calls alongside ERC20
transfers.
- Maintain compatibility with existing handlers and workflows that do
not involve contract calls, ensuring that the SDK remains functional for
all users.

## Closes: #493 

## Testing details
- **Unit Tests**: Develop unit tests that cover scenarios where both
ERC20 transfers and contract calls are involved.
- **Compatibility Tests**: Ensure that existing functionality remains
unaffected by the changes.
- **Error Handling**: Test how the SDK handles invalid or failed
contract calls within the transaction flow.

## Acceptance Criteria
- [ ] The SDK supports interactions with the new `ERC20Handler`,
allowing for both ERC20 transfers and optional contract calls within the
same transaction.
- [ ] The SDK maintains backward compatibility and continues to function
as expected with existing handlers.
- [ ] All new functionality is covered by tests, ensuring reliable and
consistent behavior.
@mpetrun5
Copy link
Collaborator

The code LGTM.
Just the tests now.

@lastperson
Copy link
Contributor Author

@mpetrun5 tests added.

@lastperson lastperson marked this pull request as ready for review September 19, 2024 18:35
@nmlinaric nmlinaric merged commit b166d41 into master Sep 20, 2024
5 checks passed
@nmlinaric nmlinaric deleted the feat/erc20ContractCall branch September 20, 2024 08:11
saadahmsiddiqui added a commit to sygmaprotocol/sygma-sdk that referenced this pull request Sep 26, 2024
#545)

# Implementation details

With [PR#266](sygmaprotocol/sygma-solidity#266),
we introduced the ability to define contract calls together with native
deposit. As SDK didn't integrate the initial implementation of native
handlers, it needs to be expanded so it can be used to create:

Deposit of native currency (tokens)
Deposit of native currency (tokens) + contract call definition
The Native flow differs slightly from regular ERC20/721/1155, as the
deposit transaction should not land on Bridge.sol but on
NativeAdapter.sol contract. We have already expanded the testnet [shared
configuration to have this as a new property of the
domain](https://github.com/sygmaprotocol/sygma-shared-configuration/blob/main/testnet/shared-config-test.json#L10)
- nativeTokenAdapter.

## Closes: #521 

# Testing details

Unit Tests: Develop unit tests that cover scenarios where both Native
transfers and contract calls are involved.
Error Handling: Test how the SDK handles invalid or failed contract
calls within the transaction flow.

# Acceptance Criteria



- [ ] The SDK supports interactions with the new
NativeAdapter->NativeHandler, allowing for both Native transfers and
optional contract calls within the same transaction.
- [ ] The SDK maintains backward compatibility and continues to function
as expected with existing handlers.
- [ ] All new functionality is covered by tests, ensuring reliable and
consistent behavior.
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.

Expand ERC20Handler with optional contract call
5 participants