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(nft): enable eth with non fungible tokens #2049

Merged
merged 48 commits into from
Mar 1, 2024

Conversation

laruh
Copy link
Member

@laruh laruh commented Jan 16, 2024

This Pull Request introduces the functionality to enable EVM based coin as a platform coin along with its associated Non-Fungible Tokens (NFTs) owned by user.
Also PR includes enable_nft implementation, which works same way as enable_erc20.

Main Features:

  1. enable_eth_with_tokens PRC: now works for both erc20 and nfts activation.
  2. New enable_nft RPC: re utilizes enable_token::<EthCoin> function
  3. NFT update in Coins Context: Ensures that the global NFT within the coins context is updated with the latest NFT information.

@laruh laruh added the in progress Changes will be made from the author label Jan 16, 2024
@laruh laruh added under review and removed in progress Changes will be made from the author labels Jan 18, 2024
Copy link
Member

@borngraced borngraced left a comment

Choose a reason for hiding this comment

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

Looks great! first iteration focusing on only some codes

mm2src/coins/eth/v2_activation.rs Outdated Show resolved Hide resolved
mm2src/coins/nft/nft_structs.rs Outdated Show resolved Hide resolved
mm2src/coins/nft.rs Outdated Show resolved Hide resolved
mm2src/coins/nft.rs Outdated Show resolved Hide resolved
mm2src/coins/eth/eth_tests.rs Outdated Show resolved Hide resolved
@laruh laruh added in progress Changes will be made from the author and removed under review labels Jan 19, 2024
@laruh
Copy link
Member Author

laruh commented Jan 19, 2024

linter fails in CI
👀 hmmm I dont have this linter error locally

error[E0434]: can't capture dynamic environment in a fn item
   --> mm2src/coins/eth/v2_activation.rs:322:17
    |
322 |             ..**self
    |                 ^^^^
    |
    = help: use the `|| { ... }` closure form instead

For more information about this error, try `rustc --explain E0434`.
error: could not compile `coins` due to previous error
warning: build failed, waiting for other jobs to finish...
Error: Process completed with exit code 101.

May be we have different env in CI, will revert this change

@laruh laruh added under review and removed in progress Changes will be made from the author labels Jan 19, 2024
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR! Only 2 comments at this stage since the first comment will lead to a lot of changes that I would rather review the code after them.

mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs Outdated Show resolved Hide resolved
mm2src/coins/eth/eth_tests.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Next review iteration! Still have one more iteration to go as I found this review is growing, please resolve conflicts also, there was a change in the web3 transport interface due to this #2058 .

mm2src/mm2_main/src/lp_swap/check_balance.rs Outdated Show resolved Hide resolved
mm2src/coins_activation/src/bch_with_tokens_activation.rs Outdated Show resolved Hide resolved
mm2src/coins/eth.rs Outdated Show resolved Hide resolved
mm2src/coins/eth.rs Show resolved Hide resolved
mm2src/coins/eth/v2_activation.rs Outdated Show resolved Hide resolved
mm2src/coins/nft.rs Outdated Show resolved Hide resolved
#[allow(clippy::result_large_err)]
pub fn coin_conf_with_protocol<T: TryFromCoinProtocol>(
ctx: &MmArc,
coin: &str,
) -> Result<(Json, T), MmError<CoinConfWithProtocolError>> {
let conf = coin_conf(ctx, coin);
let (conf, coin_protocol) = match Chain::from_nft_ticker(coin) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I was a bit wrong in suggesting not adding NFTs in coin configs, if moralis supported more EVM chains or we moved away from moralis for a web3 native implementation (if possible in the future), it would be easier to add new chains and their NFTs in config without requiring a PR in komodefi. I guess one way to do this in the future is to have a field in platform coin configuration for if they support NFT or not, and we enable global NFT using the platform ticker and append NFT_ prefix to it. This is not needed at all for now, but if it was requested in the future we can do it.

@laruh laruh force-pushed the enable-eth-non-fungible-tokens branch from 507194a to 7e98ae1 Compare February 27, 2024 16:07
…e-tokens

# Conflicts:
#	mm2src/coins/eth.rs
#	mm2src/coins/eth/v2_activation.rs
#	mm2src/coins_activation/src/erc20_token_activation.rs
#	mm2src/coins_activation/src/eth_with_token_activation.rs
@laruh
Copy link
Member Author

laruh commented Feb 27, 2024

Ive reverted the name of the erc20_token_activation module to minimize conflicts within this module.
Considering the potential for conflicts in other PRs and our uncertainty about whether well continue using the same methods for both eth and nft impls, should we stick with the old name? Alternatively, I could rename it again, perhaps to erc20_nft_token_activation.
@shamardy @onur-ozkan

@shamardy
Copy link
Collaborator

should we stick with the old name? Alternatively, I could rename it again, perhaps to erc20_nft_token_activation

We can leave the old name for now and rename it later.

shamardy
shamardy previously approved these changes Feb 28, 2024
Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

LGTM! Only a few non-blockers!

mm2src/coins/lp_coins.rs Outdated Show resolved Hide resolved
mm2src/coins/eth/v2_activation.rs Outdated Show resolved Hide resolved
@shamardy
Copy link
Collaborator

shamardy commented Mar 1, 2024

@laruh can you please provide @KomodoPlatform/qa examples for the new enable_nft RPC. You can provide it here post merge or in the NFT issue #900. @KomodoPlatform/qa I think it would be good to make sure that other NFT RPCs work well with and without activation.

@shamardy shamardy merged commit 238a0f2 into dev Mar 1, 2024
28 of 34 checks passed
@shamardy shamardy deleted the enable-eth-non-fungible-tokens branch March 1, 2024 04:31
dimxy added a commit to dimxy/komodo-defi-framework that referenced this pull request Mar 13, 2024
* dev:
  feat(indexeddb): advanced cursor filtering impl (KomodoPlatform#2066)
  update dockerhub destination repository (KomodoPlatform#2082)
  feat(event streaming): configurable worker path, use SharedWorker (KomodoPlatform#2080)
  fix(hd_tests): fix test_hd_utxo_tx_history unit test (KomodoPlatform#2078)
  feat(network): improve efficiency of known peers handling (KomodoPlatform#2074)
  feat(nft): enable eth with non fungible tokens (KomodoPlatform#2049)
  feat(ETH transport & heartbeats): various enhancements/features (KomodoPlatform#2058)
Alrighttt pushed a commit that referenced this pull request Mar 18, 2024
This commit introduces the functionality to enable EVM based coin as a platform coin along with its associated Non-Fungible Tokens (NFTs) owned by user. It includes enable_nft implementation, which works same way as enable_erc20.
Alrighttt added a commit that referenced this pull request Mar 18, 2024
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.

6 participants