Skip to content
This repository has been archived by the owner on Jul 30, 2024. It is now read-only.

Fix: wrong denom check at RegsterCoin and remove IBC-related tests #6

Merged
merged 7 commits into from
Jul 30, 2024
Merged

Fix: wrong denom check at RegsterCoin and remove IBC-related tests #6

merged 7 commits into from
Jul 30, 2024

Conversation

jasonsong0
Copy link

@jasonsong0 jasonsong0 commented Jul 29, 2024

Description

This is based on the following PRs patched in canto v8.

Problem

  1. If multiple token pairs exist for same denom, the iteration order is based on bytes, not the registration order.

  2. coinMetadata.Base is used as the denom for the token pair
    but coinMetadata.Name was used for checking registered token pair

Solution

  1. Added indexes at genesis state to track active token pair.
  2. Check registered token pair using coinMatadata.Base

Proof of Work

  1. TestCase: denom index should be same with lastest token pair id
  2. TestCase: The test case for duplicate denom registering passes with the fix in the PR

Testcode Refactoring

  • IBC-realted tests are removed because IBC features have been removed in Basechain.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • included the necessary unit and integration tests
  • reviewed "Files changed" and left comments if necessary

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@jasonsong0 jasonsong0 marked this pull request as ready for review July 29, 2024 06:46
@jasonsong0 jasonsong0 requested a review from dongsam July 29, 2024 06:46
@dongsam dongsam requested a review from zsystm July 29, 2024 06:47
Copy link

@zsystm zsystm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@dongsam dongsam left a comment

Choose a reason for hiding this comment

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

Need to Cherry-pick below PRs without canto v8(sdk 50) dependent code

Could you backport patch for b-harvest#68 or If you think a is not necessary, please leave a comment about the reason @jasonsong0

@jasonsong0
Copy link
Author

Need to Cherry-pick below PRs without canto v8(sdk 50) dependent code

Could you backport patch for b-harvest#68 and Review request again? @jasonsong0

The index was added as a patch for the existence of multiple token pairs with the same denom, which should not happen if the above PR is fixed.
The name change better serves its purpose, so I'll apply the commit.

@dongsam
Copy link
Member

dongsam commented Jul 29, 2024

@jasonsong0 It might be a good to backporting patch proceed with b-harvest#68 even after applying the duplication prevention patch b-harvest#78 for the following reasons:

  • The genesis export/import logic has been clearly modified.
  • The DenomMap(TokenPairDenomIndex) and ERC20Map(TokenPairERC20AddressIndex), which were missing in the genesis state, have been explicitly added.
  • By using common code and naming through backporting, there are benefits in terms of future maintenance.

@zsystm , If you have any opinions on this as well, Please let us know

@dongsam dongsam requested review from zsystm and dongsam July 29, 2024 11:58
@zsystm
Copy link

zsystm commented Jul 30, 2024

@jasonsong0 It might be a good to backporting patch proceed with b-harvest#68 even after applying the duplication prevention patch b-harvest#78 for the following reasons:

  • The genesis export/import logic has been clearly modified.
  • The DenomMap(TokenPairDenomIndex) and ERC20Map(TokenPairERC20AddressIndex), which were missing in the genesis state, have been explicitly added.
  • By using common code and naming through backporting, there are benefits in terms of future maintenance.

@zsystm , If you have any opinions on this as well, Please let us know

Agree with @dongsam
@jasonsong0 I think it's good to have b-harvest#68

Ah ha. You already added commits. I'll review it.

Copy link

@zsystm zsystm left a comment

Choose a reason for hiding this comment

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

@jasonsong0

  • same denom and multiple token pair bug should not happen in this codebase.
  • the previous comments could be interpreted as if this issue can already occur or has occurred, so I have revised the comment

x/erc20/genesis.go Outdated Show resolved Hide resolved
x/erc20/genesis_test.go Show resolved Hide resolved
Copy link

@zsystm zsystm left a comment

Choose a reason for hiding this comment

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

LGTM
FYI. @jasonsong0 test-coverage CI failed.

x/erc20/genesis_test.go Outdated Show resolved Hide resolved
@dongsam dongsam self-requested a review July 30, 2024 02:51
@dongsam
Copy link
Member

dongsam commented Jul 30, 2024

LGTM, Please merge it If all the work is done @jasonsong0

@jasonsong0 jasonsong0 merged commit 78a3997 into b-harvest:abci1.0/develop Jul 30, 2024
8 checks passed
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.

3 participants