feat(nft): nft abi in withdraw_nft RPC, clear_nft_db RPC by laruh · Pull Request #2039 · GLEECBTC/komodo-defi-framework

laruh · 2023-12-18T11:44:17Z

This PR introduces two new functions, `erc1155_balance` and `erc721_owner`, to the `EthCoin` structure.

These enhancements streamline the NFT withdrawal process in the withdraw_nft RPC by directly interacting with smart contracts for validation, eliminating the need to access database information.

Key Features and Benefits:

Direct Balance Check for ERC1155: The erc1155_balance function allows us to verify the current balance of ERC1155 tokens owned by a user by querying the smart contract directly.
Ownership Validation for ERC721: With erc721_owner, we can now confirm whether a user is the rightful owner of an ERC721 token at the time of the withdrawal request.
Prevent Invalid Withdrawal Requests: This update crucially enables the system to reject withdrawal requests if the user attempts to transfer an NFT using an RPC method that is incompatible with the NFT's contract type (such as using withdraw_erc1155 for an ERC721 token, and vice versa).

Technical Notes:

The ERC721 standard uniquely features the ownerOf function, unlike ERC1155.
Although both ERC721 and ERC1155 standards have a balanceOf function, they require different parameters (ERC721's balanceOf, ERC1155's balanceOf).

New `clear_nft_db` RPC for NFT data management was added.

This new method allows users to selectively clear NFT data for specified blockchain networks, or to completely wipe it across all supported chains.

Key Features:

Selective Clearing: Users can specify which blockchain networks (like Ethereum, BSC) to clear.
Global Clearing Option: With the clear_all flag set to true, the RPC clears NFT data from all chains. In this case chains parameter will be ignored.
Parameter Validation: When clear_all is false and no specific chains are provided, then the mm error occurs "Nothing to clear was specified".

onur-ozkan

First review iteration:

mm2src/coins/nft/storage/sql_storage.rs

mm2src/coins/eth.rs

mm2src/coins/nft.rs

borngraced

great work! few changes from me

borngraced · 2024-01-15T10:55:57Z

mm2src/derives/enum_from/src/lib.rs

+/// `EnumVariantList` is a procedural macro used to generate a method that returns a vector containing all variants of an enum.
+/// This macro is intended for use with simple enums (enums without associated data or complex structures).
+///
+/// ### USAGE:
+///
+/// ```rust
+/// use enum_from::EnumVariantList;
+///
+/// #[derive(EnumVariantList)]
+/// enum Chain {
+///     Avalanche,
+///     Bsc,
+///     Eth,
+///     Fantom,
+///     Polygon,
+/// }
+///
+///#[test]
+///fn test_enum_variant_list() {
+///    let all_chains = Chain::variant_list();
+///    assert_eq!(all_chains, vec![
+///        Chain::Avalanche,
+///        Chain::Bsc,
+///        Chain::Eth,
+///        Chain::Fantom,
+///        Chain::Polygon
+///    ]);
+///}
+/// ```
+#[proc_macro_derive(EnumVariantList)]
+pub fn enum_variant_list(input: TokenStream) -> TokenStream {
+    let input = parse_macro_input!(input as DeriveInput);
+    let name = input.ident;
+
+    let variants = match input.data {
+        Data::Enum(DataEnum { variants, .. }) => variants,
+        Data::Struct(_) => return CompileError::expected_enum(ENUM_VARIANT_LIST_IDENT, "struct").into(),
+        Data::Union(_) => return CompileError::expected_enum(ENUM_VARIANT_LIST_IDENT, "union").into(),
+    };
+
+    let variant_list: Vec<_> = variants.iter().map(|v| &v.ident).collect();
+
+    let expanded = quote! {
+        impl #name {
+            pub fn variant_list() -> Vec<#name> {
+                vec![ #( #name::#variant_list ),* ]
+            }
+        }
+    };
+
+    TokenStream::from(expanded)
+}
+


I don't think this macro should be In this mod by the way, the intention and name is quite different from what's expected from the mod just unlike the other macros there . maybe another mod or just create a derives mod in coins and move there

@borngraced yeah, I felt that this macro probably wont fit the lib name.
What do you think about renaming enum_from to enum_utilities so we can just leave enum_variant_list function in lib.rs and reuse all necessary already implemented types (like error) for EnumVariantList and any other future derive macro for enum.

I did this

derives │ └── enum_utilities ├── lib.rs ├── from_inner.rs ├── from_stringify.rs └── from_trait.rs

If we leave enum_from lib name, then we need to add additional lib enum_to which actually could re-utilize implementations or types from enum_from bcz we desire to work with enums. I suggest to keep enum_utilities for different enum macros.

But if you would like to have different names, I will create new lib crate.

yeah, you can rename for sure if it worth it.. what about enum_derives

cc. @shamardy

personally, I would lean towards enum_derives as it explicitly suggests that the crate is focused on deriving traits or functionalities for enums. also gives a clear indication of the crate's purpose.

yep, enum_derives is definitely a better name, changed it, thanks!

borngraced · 2024-01-15T10:59:05Z

mm2src/derives/enum_from/src/lib.rs

@@ -148,8 +203,8 @@ impl fmt::Display for MacroAttr {
 struct CompileError(String);

 impl CompileError {


you can make CompileError public if you like to use externally.

mm2src/coins/eth.rs

… statement, use if/else

onur-ozkan · 2024-01-15T15:17:23Z

mm2src/db_common/src/sqlite.rs

+    pub fn new(table_name: &str) -> SqlResult<Self> {
+        validate_table_name(table_name)?;
+        Ok(SafeTableName(table_name.to_owned()))
+    }


Currently we can create SafeTableName without this function and read it with 'instance.0' . Maybe remove "pub" from the inner value to enforce using this function, then create another function for reading it?

Good point! made inner value private and impl get_name function SafeTableName

onur-ozkan · 2024-01-16T07:17:20Z

mm2src/coins/nft/nft_errors.rs

+/// Variants:
+/// - `DbError`: Represents errors related to database operations.
+/// - `Internal`: Indicates internal errors not directly associated with database operations.
+/// - `InvalidRequest`: Used for various types of invalid requests, such as missing or contradictory parameters.


This isn't a good approach for documenting rust code. You should add the field-related docs on the fields directly.

I see, thanks for the note! I changed as many doc comments as I've noticed. If I see more, I will fix them.

onur-ozkan · 2024-01-16T07:17:49Z

mm2src/coins/nft/nft_structs.rs

+/// Parameters for withdrawing an ERC-1155 token.
+///
+/// Fields:
+/// - `chain`: The blockchain network to perform the withdrawal on.
+/// - `to`: The address to send the NFT to.
+/// - `token_address`: The address of the ERC-1155 token contract.
+/// - `token_id`: The unique identifier of the NFT to withdraw.
+/// - `amount`: Optional amount of the token to withdraw. Defaults to 1 if not specified.
+/// - `max`: If set to `true`, withdraws the maximum amount available. Overrides the `amount` field.
+/// - `fee`: Optional details for the withdrawal fee.


Same here (there are other spots too but I don't want to ping them all)

…get_name func

onur-ozkan

Last review iteration

mm2src/db_common/src/sqlite.rs

mm2src/coins/nft.rs

mm2src/coins/nft/nft_errors.rs

mm2src/coins/eth.rs

…onal

shamardy

LGTM! Some minor non-blockers!

mm2src/coins/eth.rs

mm2src/coins/nft.rs

…sError`

shamardy · 2024-02-12T15:00:45Z

@KomodoPlatform/qa Since there are a lot of open PRs, this can be tested in dev. @laruh In addition to this #2039 (comment) you can add example request/responses for the QA team here or in the related issues #900, #1902
P.S. I want to start merging as many PRs as possible since a lot of conflicts are starting to appear, so I will bypass the waiting for QA and sec team approvals on PRs for now and instead the review/approvals can be done on release PR.

* evm-hd-wallet: (27 commits) Fix todo comments Fix HDAddressOps::Address trait bounds fix(indexeddb): fix IDB cursor.continue_() call after drop (GLEECBTC#2028) security bump for `h2` (GLEECBTC#2062) fix(makerbot): allow more than one prices url in makerbot (GLEECBTC#2027) fix(wasm worker env): refactor direct usage of `window` (GLEECBTC#1953) feat(nft): nft abi in withdraw_nft RPC, clear_nft_db RPC (GLEECBTC#2039) refactor(utxo): refactor utxo output script creation (GLEECBTC#1960) feat(ETH): balance event streaming for ETH (GLEECBTC#2041) chore(release): bump mm2 version to 2.1.0-beta (GLEECBTC#2044) feat(trezor): add segwit support for withdraw with trezor (GLEECBTC#1984) chore(config): remove vscode launchjson (GLEECBTC#2040) feat(trading-proto-upgrade): wasm DB, kickstart, refund states, v2 RPCs (GLEECBTC#2015) feat(UTXO): balance event streaming for Electrum clients (GLEECBTC#2013) feat(tx): add new sign_raw_transaction rpc for UTXO and EVM coins (GLEECBTC#1930) fix(p2p): handle encode_and_sign errors (GLEECBTC#2038) chore(release): add changelog entries for v2.0.0-beta (GLEECBTC#2037) chore(network): write network information to stdout (GLEECBTC#2034) fix(price_endpoints): add cached url (GLEECBTC#2032) deps(network): sync with upstream yamux (GLEECBTC#2030) ...

* dev: feat(zcoin): ARRR WASM implementation (GLEECBTC#1957) feat(trading-proto-upgrade): locked amounts, kmd burn and other impl (GLEECBTC#2046) fix(indexeddb): set stop on success cursor condition (GLEECBTC#2067) feat(config): add `max_concurrent_connections` to mm2 config (GLEECBTC#2063) feat(stats_swaps): add gui/mm_version in stats db (GLEECBTC#2061) fix(indexeddb): fix IDB cursor.continue_() call after drop (GLEECBTC#2028) security bump for `h2` (GLEECBTC#2062) fix(makerbot): allow more than one prices url in makerbot (GLEECBTC#2027) fix(wasm worker env): refactor direct usage of `window` (GLEECBTC#1953) feat(nft): nft abi in withdraw_nft RPC, clear_nft_db RPC (GLEECBTC#2039) refactor(utxo): refactor utxo output script creation (GLEECBTC#1960) feat(ETH): balance event streaming for ETH (GLEECBTC#2041) chore(release): bump mm2 version to 2.1.0-beta (GLEECBTC#2044) feat(trezor): add segwit support for withdraw with trezor (GLEECBTC#1984)

laruh added 10 commits December 12, 2023 18:11

wip, fn erc1155_balance added

696c25f

impl erc721_owner

c19e007

clear_nft_db RPC

1586b71

EnumVariantList macro derive, clear all methods

50d2ed4

finish todo methods, add more tests

a9a68f0

return InvalidRequest at the begging

087f5da

doc comms

ab0ddc9

Merge remote-tracking branch 'origin/dev' into nft-enhancements

65cb04b

Merge remote-tracking branch 'origin/dev' into nft-enhancements

afc5ebf

update doc com

20f3057

laruh added the under review label Dec 19, 2023

onur-ozkan suggested changes Jan 10, 2024

View reviewed changes

mm2src/coins/nft/storage/sql_storage.rs Outdated Show resolved Hide resolved

mm2src/coins/nft/storage/sql_storage.rs Outdated Show resolved Hide resolved

mm2src/coins/eth.rs Outdated Show resolved Hide resolved

mm2src/coins/nft.rs Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin/dev' into nft-enhancements

9e9d185

shamardy requested a review from borngraced January 15, 2024 10:07

borngraced suggested changes Jan 15, 2024

View reviewed changes

laruh added in progress and removed under review labels Jan 15, 2024

use u256_to_big_decimal, impl struct SafeTableName, remove prepared…

d505c56

… statement, use if/else

onur-ozkan reviewed Jan 15, 2024

View reviewed changes

onur-ozkan suggested changes Jan 16, 2024

View reviewed changes

laruh added 2 commits January 16, 2024 16:27

rename enum_from to enum_utilities, doc above the item it documents, …

5aadd7c

…get_name func

reformat more doc coms in idiomatic Rust style

6acfff9

laruh added under review and removed in progress labels Jan 16, 2024

onur-ozkan suggested changes Jan 16, 2024

View reviewed changes

mm2src/db_common/src/sqlite.rs Outdated Show resolved Hide resolved

mm2src/coins/nft.rs Outdated Show resolved Hide resolved

mm2src/coins/nft/nft_errors.rs Show resolved Hide resolved

mm2src/coins/eth.rs Outdated Show resolved Hide resolved

rename to enum_derives, fix doc comm, fn inner, vec chains non-opti…

32fc16e

…onal

shamardy previously approved these changes Feb 2, 2024

View reviewed changes

mm2src/coins/eth.rs Outdated Show resolved Hide resolved

mm2src/coins/eth.rs Outdated Show resolved Hide resolved

mm2src/coins/nft.rs Show resolved Hide resolved

avoid code nesting in clear_nft_db, remove map_err for `NumConver…

21f67b4

…sError`

laruh dismissed shamardy’s stale review via 21f67b4 February 5, 2024 12:19

shamardy approved these changes Feb 12, 2024

View reviewed changes

shamardy added QA labels Feb 12, 2024

shamardy merged commit f025652 into dev Feb 12, 2024

shamardy deleted the nft-enhancements branch February 12, 2024 15:39

shamardy mentioned this pull request Aug 1, 2024

chore(release): v2.1.0-beta #2043

Merged

		@@ -148,8 +203,8 @@ impl fmt::Display for MacroAttr {
		struct CompileError(String);

		impl CompileError {

Conversation

laruh commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This PR introduces two new functions, erc1155_balance and erc721_owner, to the EthCoin structure.

New clear_nft_db RPC for NFT data management was added.

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

borngraced left a comment

Choose a reason for hiding this comment

Uh oh!

borngraced Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laruh Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laruh Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borngraced Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

borngraced Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

laruh Jan 17, 2024

Choose a reason for hiding this comment

Uh oh!

borngraced Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

onur-ozkan Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laruh Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

onur-ozkan Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

laruh Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

onur-ozkan Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shamardy commented Feb 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

laruh commented Dec 18, 2023 •

edited

Loading

This PR introduces two new functions, `erc1155_balance` and `erc721_owner`, to the `EthCoin` structure.

New `clear_nft_db` RPC for NFT data management was added.

borngraced Jan 15, 2024 •

edited

Loading

laruh Jan 16, 2024 •

edited

Loading

laruh Jan 16, 2024 •

edited

Loading

borngraced Jan 15, 2024 •

edited

Loading

onur-ozkan Jan 15, 2024 •

edited

Loading

shamardy commented Feb 12, 2024 •

edited

Loading