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.

Sign up for GitHub

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

Jump to bottom

fix(nft): add token_id field to the tx history primary key, fix balance #2209

Merged
merged 23 commits into from
Nov 6, 2024
Merged

fix(nft): add token_id field to the tx history primary key, fix balance #2209

merged 23 commits into from
Nov 6, 2024

Conversation

laruh
Copy link
Member

@laruh laruh commented Sep 3, 2024 •
edited by shamardy
Loading

fixes #2208

also in this commit 2bdee4f fixes withdarw_erc1155 decimal issue. ERC1155 "balanceOf" method started returning exact Nft amount owned by wallet without any scaling, so we dont need to use self.decimals shift when we convert U256 type to BigDecimal

To Test:
Already tested by @smk762 #2209 (review) but it would be good to make sure everything still works fine after the review comments fixes then close this issue #2208

@laruh laruh requested a review from smk762 September 3, 2024 02:57
@laruh laruh added under review in progress Changes will be made from the author and removed under review labels Sep 3, 2024
@laruh laruh added under review and removed in progress Changes will be made from the author labels Sep 3, 2024
@laruh
fix erc1155_balance, check that "balanceOf" ERC11155 returns the exac…
2bdee4f 
…t amount of token without any decimals or scaling factors
@laruh laruh changed the title fix(nft_db): add token_id field to the tx history primary key fix(nft): add token_id field to the tx history primary key, fix balanceOf Sep 3, 2024
@laruh laruh changed the title fix(nft): add token_id field to the tx history primary key, fix balanceOf fix(nft): add token_id field to the tx history primary key, fix balance Sep 3, 2024
smk762
smk762 previously approved these changes Sep 5, 2024
View reviewed changes
Copy link

@smk762 smk762 left a comment

Choose a reason for hiding this comment

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

Thanks, NFT history and 1155 withdraws confirmed working.

Note: Withdraws of a spammy NFT were throwing gas errrors, but a good NFT was fine. I assume the spammy one was trying to scalp gas.

shamardy
shamardy reviewed Sep 6, 2024
View reviewed changes
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 for the fix! First review iteration!

mm2src/coins/eth.rs Outdated Show resolved Hide resolved
mm2src/coins/nft/storage/sql_storage.rs Show resolved Hide resolved
@laruh laruh added in progress Changes will be made from the author and removed under review labels Sep 8, 2024
@laruh laruh force-pushed the fix-nft-tx-history-primkey branch 2 times, most recently from 44ca4a5 to a5c612c Compare September 15, 2024 10:57
@laruh laruh added under review and removed in progress Changes will be made from the author labels Oct 14, 2024
@laruh
Copy link
Member Author

laruh commented Oct 14, 2024 •
edited
Loading

@borngraced @shamardy
All is good on IndexedDB side in our on_upgrade_needed function implementations from TableSignature trait.
Found out that when adding a new unique index during a database upgrade, IndexedDB automatically updates it with the existing records, eliminating the need for manual re-indexing.

We have pretty good our own representation of https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API with low-level Rust wrappers over basic JS structures, so I suggest to make this crate as a separate repo (during kdf refactoring) and deploy on crate.io. So we can present komodo-idb-rs crate. There is small number of developed indexeddb libs for rust, which is good opportunity for us.

I remember about the need to create an issue about new db clients suggestion for sql part, so also will add komodo idb crate note to it.

borngraced
borngraced reviewed Oct 14, 2024
View reviewed changes
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.

Great work!! just couple questions and I will approve

mm2src/coins/nft/storage/wasm/wasm_storage.rs Outdated Show resolved Hide resolved
mm2src/coins/nft/storage/wasm/wasm_storage.rs Outdated Show resolved Hide resolved
@laruh
Copy link
Member Author

laruh commented Oct 15, 2024 •
edited
Loading

Created two issues about sql db migration tools #2243 and separate indexed db repo #2244

borngraced
borngraced previously approved these changes Oct 15, 2024
View reviewed changes
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.

Great work! wasm code looks great

shamardy
shamardy reviewed Oct 24, 2024
View reviewed changes
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 for the fixes! Next review iteration!

mm2src/coins/eth.rs Outdated Show resolved Hide resolved
mm2src/coins/nft/storage/wasm/wasm_storage.rs Outdated Show resolved Hide resolved
mm2src/coins/nft/storage/sql_storage.rs Outdated Show resolved Hide resolved
@laruh laruh added in progress Changes will be made from the author and removed under review labels Oct 26, 2024
@laruh laruh added under review and removed in progress Changes will be made from the author labels Oct 28, 2024
shamardy
shamardy approved these changes Nov 6, 2024
View reviewed changes
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 fixes!

@shamardy shamardy merged commit a1b02c1 into dev Nov 6, 2024
20 of 23 checks passed
@shamardy shamardy deleted the fix-nft-tx-history-primkey branch November 6, 2024 15:43
@shamardy shamardy added the QA label Nov 6, 2024
dimxy added a commit that referenced this pull request Nov 7, 2024
@dimxy
Merge branch 'dev' into add-eth-access-list
bb9ca53 
* dev:
  fix(nft): add token_id field to the tx history primary key, fix balance (#2209)
  feat(cosmos): support IBC types in tx history implementation (#2245)
  fix(hd-wallet): use `CoinBalanceMap` for UTXO and QTUM (#2259)
  fix(tests): add more sepolia endpoints in tests (#2262)
  fix(legacy-swap): check for confirmations on recover taker (#2242)
  fix(legacy-swap): remove the need for takers to confirm their payment (#2249)
  refactor(P2P): types and modules (#2256)
  fix(evm): correctly display eth addr in iguana v2 activation result (#2254)
  feat(utxo): prioritize electrum connections (#1966)
  refactor(SwapOps): make all methods async (#2251)
  refactor(SwapOps): make `send_maker_payment` async (#2250)
  remove old p2p implementation (#2248)
dimxy added a commit that referenced this pull request Nov 11, 2024
@dimxy
Merge branch 'dev' into 1inch-classic-swap
57efd04 
* dev:
  fix(foot-shooting): remove leftover code that panics via RPC (#2270)
  refactor(MarketCoinOps): make `wait_for_htlc_tx_spend` async (#2265)
  feat(eth-swap): maker tpu v2 implementation (#2211)
  fix(nft): add token_id field to the tx history primary key, fix balance (#2209)
  feat(cosmos): support IBC types in tx history implementation (#2245)
  fix(hd-wallet): use `CoinBalanceMap` for UTXO and QTUM (#2259)
  fix(tests): add more sepolia endpoints in tests (#2262)
  fix(legacy-swap): check for confirmations on recover taker (#2242)
  fix(legacy-swap): remove the need for takers to confirm their payment (#2249)
  refactor(P2P): types and modules (#2256)
  fix(evm): correctly display eth addr in iguana v2 activation result (#2254)
  feat(utxo): prioritize electrum connections (#1966)
  refactor(SwapOps): make all methods async (#2251)
  refactor(SwapOps): make `send_maker_payment` async (#2250)
  remove old p2p implementation (#2248)
  feat(cosmos-offline-tests): prepare IBC channels inside the container  (#2246)
@shamardy shamardy mentioned this pull request Nov 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shamardy shamardy shamardy approved these changes

@borngraced borngraced borngraced left review comments

@smk762 smk762 smk762 left review comments

Assignees
No one assigned
Labels
QA under review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@laruh @shamardy @borngraced @smk762