Change to multitokens #1693

yito88 · 2023-07-10T09:09:59Z

Reopen #1663

Renew balance keys with Multitoken
- Account balance key: #Multitoken/{token_addr}/balance/{owner}
- Supply balance key: #Multitoken/{token_addr}/balance/minted
Add Multitoken VP shared/src/ledger/native_vp/multitoken.rs
Remove sub_prefix
- Change printing balances
IBC: Clean up with Multitokens
- IBC-related balance key: #Multitoken/#IbcToken(hash)/balance/{owner}
- Delete IbcToken VP since Multitoken VP checks the balance changes
EthBridge: Change the balance keys
- ERC20 balance key: #Multitoken/#Erc20(eth_addr)/balance/{owner}
- ERC20 supply key: #Multitoken/#Erc20(eth_addr)/balance/minted
- Remove redundancy from EthBridge/EthBridgePool VP after adding Multitoken VP

sug0

some changes + comments

core/src/ledger/ibc/mod.rs

apps/src/lib/node/ledger/shell/finalize_block.rs

apps/src/lib/client/rpc.rs

core/src/ledger/eth_bridge/storage/wrapped_erc20s.rs

shared/src/ledger/queries/shell/eth_bridge.rs

ethereum_bridge/src/protocol/transactions/ethereum_events/mod.rs

shared/src/ledger/native_vp/multitoken.rs

shared/src/ledger/protocol/mod.rs

wasm/wasm_source/src/tx_bridge_pool.rs

sug0 · 2023-07-11T08:43:38Z

@yito88 could you also update the supply key of NAM, as part of this PR, when we transfer NAM to Ethereum or receive wNAM from Ethereum, in ethereum_bridge/src/protocol/transactions/ethereum_events/events.rs?

since we're escrowing/descrowing NAM from the Ethereum bridge address, the total supply doesn't change. nevermind the comment above ^^^^

sug0

LGTM

sug0 · 2023-07-11T15:48:54Z

don't merge before CI passes though :D

yito88 · 2023-07-12T16:43:47Z

Updated for removing vp_token since the validation was moved to Multitoken VP

cwgoes · 2023-07-14T08:17:42Z

Are these CI E2E failures expected?

batconjurer · 2023-07-14T08:19:35Z

Are these CI E2E failures expected?

The masp one is. I don't know about the gensis validators test though. It wasn't failing before IIRC

cwgoes

(just re-reviewed the multitoken VP part)

cwgoes · 2023-07-14T08:59:34Z

shared/src/ledger/native_vp/multitoken.rs

+                let post: Amount = self.ctx.read_post(key)?.unwrap_or_default();
+                let diff = post.change() - pre.change();
+                if diff.is_negative()
+                    && !(verifiers.contains(owner) || *owner == address::masp())


Why do we have to check specifically for the MASP? If the MASP balance is decremented, the MASP address should be a verifier, no?

You are right. When the owner is the MASP address, the verifiers should have the MASP address.
And, for any other owner, the verifiers should have the address, too.
This check isn't needed anymore though I moved this from vp_token. Thanks.

wait, why did you remove the check completely? don't we still need to check that the owner of any decremented balance approved the transaction?

Because that is checked by vp_user.

namada/wasm/wasm_source/src/vp_user.rs

Lines 91 to 92 in cd95787

let valid =

change.non_negative() || addr == masp() || *valid_sig;

sug0 · 2023-07-14T09:24:35Z

Are these CI E2E failures expected?

The masp one is. I don't know about the gensis validators test though. It wasn't failing before IIRC

Test genesis validators is expected to fail on a v0.19.0 base. It's fixed in the upcoming release.

yito88 · 2023-07-14T09:46:21Z

Thank you. It looks like test_genesis_validators fails on v0.19.0.
#1698

* origin/yuji/ibc-multitoken: remove test_invalid_sender remove negative check remove vp_token fix client_connections encoding add comments fix according to feedback fix a test fix for tx signing for clippy fix multitoken vp to check the minter for clippy modify EthBridge VP and EthBridgePool VP for multitoken revert unexpected changes change eth_bridge balance keys to multitoken keys add unit tests remove InternalAddress::Minted rename fix token decoding fix balance print add multitoken VP file change to multitokens change multitoken clean denom validation

tzemanovic · 2023-07-21T10:22:41Z

There's an actual issue e2e::ledger_tests::masp_txs_and_queries in here that's still happening with the changes in draft. It's reliably failing on a crash of tx_transfer wasm in a shielded to shielded transfer

sug0 · 2023-07-21T11:01:17Z

@tzemanovic that sounds very similar to the issue me and @batconjurer experienced a few weeks ago. could it be that the wasm sizes have gone up again, and caused the vm to crash? can you try our fix that consisted of adding a moderately large stack allocation somewhere, with a volatile read to its memory?

yito88 · 2023-07-21T19:08:05Z

I fixed the issue.
e2e::ledger_tests::masp_txs_and_queries failed due to my tx_transfer change.
When a transfer from shielded to shielded, the source balance was checked, but the balance didn't exist. But we don't need to check it because we shouldn't change these balances in this case.
After refactoring tx_transfer with multitoken by removing IbcBurn/IbcMint addresses, the wrong check happened.

* origin/yuji/ibc-multitoken: fix tx_transfer changelog: add #1693 remove test_invalid_sender remove negative check remove vp_token fix client_connections encoding add comments fix according to feedback fix a test fix for tx signing for clippy fix multitoken vp to check the minter for clippy modify EthBridge VP and EthBridgePool VP for multitoken revert unexpected changes change eth_bridge balance keys to multitoken keys add unit tests remove InternalAddress::Minted rename fix token decoding fix balance print add multitoken VP file change to multitokens change multitoken clean denom validation

Namada 0.20.0 is a minor release addressing several improvements to the PoS system and the ledger stability. * tag 'v0.20.0': (113 commits) Namada 0.20.0 refator e2e masp tests fix tx_transfer ci: fix github colon parsing changelog: add #1693 channgelog: add #1738 [feat]: Moved cli commands common to testing, cli, and sdk out into apps ci: pre-download masp parameters changelog: add #1733 pos/epoched: keep 2 past epochs of data by default changelog: add #1729 test/ledger/finalize_block: fix slashing tests pos/slash: fix the validator state update to be in sync with set changes app/ledger/finalize_block: log block rewards before recording slashes test/e2e/slashing: extend the test to discover rewards issues pos: error out if validator is not in expected state for rewards changelog: add #1717 test/shell/finalize_block: add some txs to DB commit test refactor: rename function refactor: rename method remove test_invalid_sender remove negative check ci: clean up fix formatting don't use flaky sleep refactor creation of namada nodes make Who clonable refactor: use immutable reference refactor: use immutable reference refactor: remove duplicated code test/e2e/slashing: wait for wasm on original validator add changelog fix changes in finalize_block remove vp_token changelog: add #1173 client/rpc: use the new token balance method shared/ledger/queries: add token balance client-only method changelog: add #1692 changelog: add #1670 changelog: add #1667 [fix]: Fixing errors introduced by merging in main [chore]: Incorporating in review comments Update core/src/types/uint.rs Update apps/src/lib/config/genesis.rs [fix]: Fixed the faucet to use uint instead of amount. This makes it portable across different assets converts faucet_withdrawal_limit to be correct changelog: add #1656 pos: return sorted validator sets and code re-use for queries Expanding and fixing slashes query CLI query a validator's state query bonded-stake and order fix client_connections encoding handle errors for unjail-validator tx in the client expand and fix e2e test `double_signing_gets_slashed` add unjail tx at CLI changelog: add #1621 ci/test/e2e: add new test apps: move epoch-sleep client cmd under utils changelog: add #1656 pos: return sorted validator sets and code re-use for queries Expanding and fixing slashes query CLI query a validator's state update pr template test/e2e/proposal_submission: wait for proposal to be committed test/e2e/double_signing: path for validator copy that keeps logs in CI add comments fix according to feedback refactor: fix formatting add changelog refactor: simplify code refactor: introduce name for magic constant add test case feat: store total consensus stake; garbage collect validator sets query bonded-stake and order wait for wasm-precompile before expecting block hash changelog: add #1605 apps: use fd-lock instead of file-lock for cross-platform support changelog: add #1695 deps: update sysinfo to latest 0.29.4 improve error handling for `set_initial_validators` changelog: add #1686 app/ledger/finalize_block: refactor validator set update for re-use app/ledger/init_chain: get genesis validator set using the new fn pos: add a function to get genesis validator consensus set for TM fix a test fix for tx signing for clippy fix multitoken vp to check the minter for clippy modify EthBridge VP and EthBridgePool VP for multitoken revert unexpected changes change eth_bridge balance keys to multitoken keys add unit tests remove InternalAddress::Minted rename fix token decoding fix balance print add multitoken VP file change to multitokens change multitoken ...

yito88 added 16 commits July 10, 2023 10:23

clean denom validation

cfdf73a

change multitoken

0e5d22b

change to multitokens

8f62edd

add multitoken VP file

fc5eab4

fix balance print

9995a20

fix token decoding

db117cf

rename

818cced

remove InternalAddress::Minted

9759185

add unit tests

aa0cbba

change eth_bridge balance keys to multitoken keys

eeb09af

revert unexpected changes

ba65482

modify EthBridge VP and EthBridgePool VP for multitoken

381dc76

for clippy

c441ad5

fix multitoken vp to check the minter

d46513f

for clippy

13f66dc

fix for tx signing

74eee2d

yito88 requested review from sug0, batconjurer and cwgoes July 10, 2023 09:10

fix a test

713dee1

sug0 requested changes Jul 10, 2023

View reviewed changes

yito88 added 2 commits July 10, 2023 21:42

fix according to feedback

2a908a0

add comments

93b6679

sug0 self-requested a review July 11, 2023 15:47

sug0 previously approved these changes Jul 11, 2023

View reviewed changes

fix client_connections encoding

079d5c1

This was referenced Jul 11, 2023

Consolidate IBC and wrapped ERC20 multitoken handling code #735

Closed

Allow the Bridge pool fees to be payed in more token types #1704

Closed

sug0 mentioned this pull request Jul 13, 2023

Implement the Ethereum token whitelist logic in Namada #1718

Closed

cwgoes requested changes Jul 14, 2023

View reviewed changes

remove negative check

749a8ec

yito88 requested a review from cwgoes July 14, 2023 09:49

remove test_invalid_sender

cd95787

cwgoes previously approved these changes Jul 14, 2023

View reviewed changes

yito88 mentioned this pull request Jul 14, 2023

Modify verifiers for Multitoken changes anoma/namada-docs#78

Closed

sug0 mentioned this pull request Jul 14, 2023

Remove native VP ADDR associated type #1724

Closed

yito88 mentioned this pull request Jul 19, 2023

Upgrade ibc-rs to 0.41.0 #1737

Merged

2 tasks

changelog: add #1693

331cde0

tzemanovic dismissed cwgoes’s stale review via 331cde0 July 19, 2023 15:07

tzemanovic added a commit that referenced this pull request Jul 19, 2023

fixup! Merge branch 'origin/yuji/ibc-multitoken' (#1693)

23b4af4

tzemanovic mentioned this pull request Jul 19, 2023

alternative draft 0.20 #1740

Closed

Fraccaman mentioned this pull request Jul 20, 2023

Alternative draft + multisig #1742

Closed

fix tx_transfer

ef2678c

Fraccaman merged commit 61ef516 into main Jul 22, 2023
10 of 12 checks passed

Fraccaman deleted the yuji/ibc-multitoken branch July 22, 2023 11:41

yito88 mentioned this pull request Jul 24, 2023

IBC Token VP should check if IBC-related data has been changed #1460

Closed

yito88 mentioned this pull request Aug 29, 2023

IBC: Denom validation is too strict #1453

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change to multitokens #1693

Change to multitokens #1693

yito88 commented Jul 10, 2023

sug0 left a comment

sug0 commented Jul 11, 2023 •

edited

Loading

sug0 left a comment

sug0 commented Jul 11, 2023

yito88 commented Jul 12, 2023

cwgoes commented Jul 14, 2023

batconjurer commented Jul 14, 2023

cwgoes left a comment

cwgoes Jul 14, 2023

yito88 Jul 14, 2023

cwgoes Jul 14, 2023

yito88 Jul 14, 2023

sug0 commented Jul 14, 2023

yito88 commented Jul 14, 2023

tzemanovic commented Jul 21, 2023

sug0 commented Jul 21, 2023

yito88 commented Jul 21, 2023

	let valid =
	change.non_negative() \|\| addr == masp() \|\| *valid_sig;

Change to multitokens #1693

Change to multitokens #1693

Conversation

yito88 commented Jul 10, 2023

sug0 left a comment

Choose a reason for hiding this comment

sug0 commented Jul 11, 2023 • edited Loading

sug0 left a comment

Choose a reason for hiding this comment

sug0 commented Jul 11, 2023

yito88 commented Jul 12, 2023

cwgoes commented Jul 14, 2023

batconjurer commented Jul 14, 2023

cwgoes left a comment

Choose a reason for hiding this comment

cwgoes Jul 14, 2023

Choose a reason for hiding this comment

yito88 Jul 14, 2023

Choose a reason for hiding this comment

cwgoes Jul 14, 2023

Choose a reason for hiding this comment

yito88 Jul 14, 2023

Choose a reason for hiding this comment

sug0 commented Jul 14, 2023

yito88 commented Jul 14, 2023

tzemanovic commented Jul 21, 2023

sug0 commented Jul 21, 2023

yito88 commented Jul 21, 2023

sug0 commented Jul 11, 2023 •

edited

Loading