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

add IBC support for 0x addresses #2051

Merged
merged 7 commits into from
Jan 24, 2025
Merged

Conversation

dssei
Copy link
Contributor

@dssei dssei commented Jan 22, 2025

Describe your changes and provide context

add IBC support for 0x addresses.
This implementation will parse the address string and if its a 0x address, will return either associated or casted Sei Address.

Testing performed to validate your change

  • unit tests

Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.30%. Comparing base (18b7597) to head (6520ead).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2051      +/-   ##
==========================================
- Coverage   61.32%   61.30%   -0.03%     
==========================================
  Files         264      264              
  Lines       24581    24586       +5     
==========================================
- Hits        15074    15072       -2     
- Misses       8378     8387       +9     
+ Partials     1129     1127       -2     
Files with missing lines Coverage Δ
app/app.go 66.37% <100.00%> (+0.02%) ⬆️
precompiles/ibc/ibc.go 47.38% <100.00%> (+0.60%) ⬆️
x/evm/keeper/address.go 88.40% <100.00%> (+2.44%) ⬆️

... and 3 files with indirect coverage changes

@dssei dssei requested a review from Kbhat1 January 22, 2025 23:43
func (h EvmAddressHandler) GetSeiAddressFromString(ctx sdk.Context, address string) (sdk.AccAddress, error) {
if common.IsHexAddress(address) {
parsedAddress := common.HexToAddress(address)
return h.evmKeeper.GetSeiAddressOrDefault(ctx, parsedAddress), nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@codchen is assumption correct here that if address is not associated yet, the funds would go to an "escrow" account and then will be migrated after association?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upd: Was able to test scenario e2e and it works

Copy link
Contributor

Choose a reason for hiding this comment

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

yes this is correct

if err != nil {
return nil, err
if !ok || receiverAddressString == "" {
return nil, errors.New("receiverAddress is not a string or empty")
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to add checks that the address is valid? i.e.

  1. if sei, it is proper bech32
  2. if evm, it is a valid address

Copy link
Contributor Author

@dssei dssei Jan 23, 2025

Choose a reason for hiding this comment

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

We did have the bech32 check before, but i was too restrictive. In fact this address is propagated as string all the way up the stack without any checks. I guess the assumption is that it could any type of address, not only bech32 or EVM.

@dssei dssei merged commit acf122d into main Jan 24, 2025
49 checks passed
@dssei dssei deleted the support_evm_address_for_incoming_ibc branch January 24, 2025 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants