Update `eth-bridge-integration` to v0.8.1 #704

james-chf · 2022-10-27T17:04:51Z

Closes #673

This PR merges v0.8.1 into eth-bridge-integration, and fixes things up.

This PR also merges #694 to fix a wasm test that was failing in eth-bridge-integration, so that CI can pass on this PR

Prebuilt .anoma that works with this PR:
local.32dd1d2fd99d3dcdb3c159cd.prebuilt.tar.gz

…y won't start however

This inverts the wrapping of errors, storage_api::Error now wraps NativeVp:Error and some other IBC and PoS custom errors

…terop/ethbridge/merge-0.8.1 * james/mainline/dont-spawn-internal-account-vps: Remove comment Update spawn_accounts to ignore implicit addresses Add changelog TestTxEnv::spawn_accounts should ignore internal addresses

james-chf · 2022-10-28T16:05:53Z

pls update wasm

This reverts commit 91f2fcb.

james-chf · 2022-10-31T17:44:35Z

pls update wasm

batconjurer · 2022-11-01T09:52:37Z

Cargo.toml

 # TODO temp patch for <https://github.com/near/borsh-rs/issues/82>, <https://github.com/near/borsh-rs/issues/84> and more tba.
 borsh = {git = "https://github.com/heliaxdev/borsh-rs.git", rev = "cd5223e5103c4f139e0c54cf8259b7ec5ec4073a"}
 borsh-derive = {git = "https://github.com/heliaxdev/borsh-rs.git", rev = "cd5223e5103c4f139e0c54cf8259b7ec5ec4073a"}
 borsh-derive-internal = {git = "https://github.com/heliaxdev/borsh-rs.git", rev = "cd5223e5103c4f139e0c54cf8259b7ec5ec4073a"}
 borsh-schema-derive-internal = {git = "https://github.com/heliaxdev/borsh-rs.git", rev = "cd5223e5103c4f139e0c54cf8259b7ec5ec4073a"}
+# The following 3 crates patch a work-around for https://github.com/smol-rs/polling/issues/38 breaking namada tooling build with nightly 2022-05-20
+polling = {git = "https://github.com/heliaxdev/polling.git", rev = "02a655775282879459a3460e2646b60c005bca2c"}


I'm so unhappy that this is how they decided to deal with this issue. If we had just frozen the minor version, we wouldn't need such a heavy handed workaround

yeah it's a bit hacky

oh this can actually be dropped now since we removed the intent gossiper/matchmaker with libp2p that was pulling these in

batconjurer · 2022-11-01T09:53:31Z

apps/Cargo.toml

 ark-serialize = "0.3.0"
 ark-std = "0.3.0"
+# branch = "bat/arse-merkle-tree"
+arse-merkle-tree = {package = "sparse-merkle-tree", git = "https://github.com/heliaxdev/sparse-merkle-tree", rev = "04ad1eeb28901b57a7599bbe433b3822965dabe8", features = ["std", "borsh"]}


alphabetization stronk

yeah we should actually have this as a lint or something on our Cargo.tomls. Would reduce merge conflicts in the future

batconjurer · 2022-11-01T09:56:39Z

apps/src/bin/anoma-wallet/cli.rs

@@ -191,17 +191,36 @@ fn address_list(ctx: Context) {
    }
 }

-/// Find address by its alias.
-fn address_find(ctx: Context, args: args::AddressFind) {
+/// Find address (alias) by its alias (address).


perplexing docstring

batconjurer · 2022-11-01T10:03:11Z

apps/src/lib/client/utils.rs

@@ -345,10 +333,31 @@ pub async fn join_network(
        .await
        .unwrap();
    }
+    if !dont_prefetch_wasm {


Double negatives. Tsk tsk

Yeah we could have a style guide around this.

batconjurer · 2022-11-01T10:09:43Z

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

 use namada::types::transaction::protocol::ProtocolTxType;

-use super::queries::QueriesExt;
+use super::governance::execute_governance_proposals;


super::* should come first

This actually gets reordered by rustfmt if I try to put super::* first. We should update the rustfmt settings if we want to enforce this

generally, anything that does a star import should come first in the list of imports, IMO

Sure, we could PR a suggestion to change rustfmt settings in main to do this

sug0

left a few comments. generally, I think it's fine to merge this, but some things should be changed after the merge IMO

sug0 · 2022-11-02T08:49:04Z

tests/src/e2e/ledger_tests.rs

@@ -56,7 +55,7 @@ fn run_ledger() -> Result<()> {
        let mut ledger =
            run_as!(test, Who::NonValidator, Bin::Node, args, Some(40))?;
        ledger.exp_string("Anoma ledger node started")?;
-        ledger.exp_string("This node is a fullnode")?;
+        ledger.exp_string("This node is not a validator")?;


I preferred This node is a fullnode

no preference though we should have some helper func or something to centralize this check, atm these lines are duplicated in a few places

sug0 · 2022-11-02T08:50:27Z

tests/src/e2e/ledger_tests.rs

+    client.exp_string(
+        "Invalid proposal end epoch: difference between proposal start and \
+         end epoch must be at least 3 and at max 27 and end epoch must be a \
+         multiple of 3",
+    )?;


inb4 this test fails in the future because this specific log string changes

Yeah we shouldn't be testing by asserting on log strings imo. Right now this is the way it currently is though

sug0 · 2022-11-02T08:55:14Z

tests/src/e2e/ledger_tests.rs

+    validator_0_copy.exp_string("Anoma ledger node started")?;
+    validator_0_copy.exp_string("This node is a validator")?;
+    let _bg_validator_0_copy = validator_0_copy.background();
+
+    // 5. Submit a valid token transfer tx to validator 0
+    let validator_one_rpc = get_actor_rpc(&test, &Who::Validator(0));
+    let tx_args = [
+        "transfer",
+        "--source",
+        BERTHA,
+        "--target",
+        ALBERT,
+        "--token",
+        XAN,
+        "--amount",
+        "10.1",
+        "--fee-amount",
+        "0",
+        "--gas-limit",
+        "0",
+        "--fee-token",
+        XAN,
+        "--ledger-address",
+        &validator_one_rpc,
+    ];


I think this code allows you to run into #644

we should check if the rpc server has started, no?

sug0 · 2022-11-02T09:00:18Z

tests/src/e2e/ledger_tests.rs

+    let mut client = run!(test, Bin::Client, tx_args, Some(40))?;
+    client.exp_string("Transaction is valid.")?;
+    client.assert_success();


should be checking if the tx was applied. Transaction is valid gets logged for accepted and applied txs

sug0 · 2022-11-02T09:07:00Z

shared/src/ledger/storage_api/collections/lazy_map.rs

+pub struct LazyMap<K, V, SON = super::Simple> {
+    key: storage::Key,
+    phantom_k: PhantomData<K>,
+    phantom_v: PhantomData<V>,
+    phantom_son: PhantomData<SON>,
+}


nice. this will be useful for some refactors

sug0 · 2022-11-02T09:16:38Z

apps/src/bin/anoma-client/cli.rs

-                }
-                Sub::SubscribeTopic(SubscribeTopic(args)) => {
-                    gossip::subscribe_topic(ctx, args).await;
-                }
            }
        }
        cli::AnomaClient::WithoutContext(cmd, global_args) => match cmd {


Suggested change

cli::AnomaClient::WithoutContext(cmd, global_args) => match cmd {

cli::NamadaClient::WithoutContext(cmd, global_args) => match cmd {

eventually we should update these idents

sug0 · 2022-11-02T09:33:59Z

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

    use namada::types::transaction::Fee;
    use tempfile::tempdir;
    use tokio::sync::mpsc::{Sender, UnboundedReceiver};

    use super::*;
+    #[cfg(feature = "abciplus")]


Suggested change

#[cfg(feature = "abciplus")]

#[cfg(not(feature = "abcipp"))]

we should follow the general pattern of the shims. we don't use abciplus anywhere else I don't think

sug0 · 2022-11-02T09:37:06Z

apps/src/lib/node/ledger/tendermint_node.rs

@@ -28,7 +30,7 @@ pub const ENV_VAR_TM_STDOUT: &str = "ANOMA_TM_STDOUT";

 #[cfg(feature = "abciplus")]
 pub const VERSION_REQUIREMENTS: &str = ">= 0.37.0-alpha.2, <0.38.0";
-#[cfg(feature = "abcipp")]
+#[cfg(not(feature = "abciplus"))]


same thing here

sug0 and others added 30 commits September 6, 2022 14:13

WIP: Shimming main

69d752a

Shim PrepareProposal

84a6479

Shim ProcessProposal

6d62289

Shim queries

c05d320

WIP: Shimming main

1f0a56f

Remove unused feature flag from Makefile

7b8260b

Cargo lock nonsense

586f3e9

WIP: Shimming main

6eafbb0

More fixes

a0e4df8

Add ConsensusParams import

aeeb94e

Find and replace Tendermint imports

9df7ab5

Add Cargo.lock file diffs

6724815

Fix abciplus

44d52dd

Add missing shell methods

6b53113

Fix abcipp

26154f1

Remaining fixes

a828214

Removed tm events log, put websocket back everywhere in client

3e5673a

Add Cargo.lock changes

00fc541

deps: replace hex with data-encoding

b8eccf3

shared/storage/key: add support for int/uint keys that maintain order

b47d1c1

test/vm_host_env: check prefix iter order in tx and VP

e648085

add support for rev_iter_prefix in storage and VP and tx envs

1429b92

tests: extend prefix iter tests for reverse order

881c93c

changelog: add #458

d786a4a

[ci skip] wasm checksums update

1d2f1dd

Change the way we call Tendermint

74d83f2

Updated the lock file and fixed resulting issues. Tendermint currentl…

6f5cd41

…y won't start however

Fix websockets and parsing of events returned from Tm

0f75154

ledger: use storage_api::Error in VpEnv and TxEnv instead of generic

a700e49

This inverts the wrapping of errors, storage_api::Error now wraps NativeVp:Error and some other IBC and PoS custom errors

[ci skip] wasm checksums update

45dddbe

james-chf force-pushed the interop/ethbridge/merge-0.8.1 branch from eab5cad to bd1cdd8 Compare October 28, 2022 13:06

james-chf added 5 commits October 28, 2022 15:30

Fix clippy

1333e6e

Fix wasms

768948e

Run make fmt

73e03b4

Temporarily disable some e2e tests

91f2fcb

github-actions bot and others added 4 commits October 28, 2022 16:06

[ci] wasm checksums update

1f7b94f

Revert "Temporarily disable some e2e tests"

cec75fb

This reverts commit 91f2fcb.

Rebuilt test wasms

788d407

write_validator_eth_*_key methods should return a Result

337ed2d

james-chf mentioned this pull request Oct 31, 2022

Temporary branch to investigate e2e test failures while upgrading eth-bridge-integration to v0.8.1 #716

Closed

james-chf added 2 commits October 31, 2022 16:42

Don't double serialize eth keys

8e66480

Disable ethbridge in double_signing_gets_slashed test

fc70c76

Update wasm/checksums.json

8ae9563

james-chf force-pushed the interop/ethbridge/merge-0.8.1 branch from 4e26648 to 8ae9563 Compare October 31, 2022 17:45

james-chf marked this pull request as ready for review October 31, 2022 18:09

james-chf requested review from Fraccaman, batconjurer and sug0 October 31, 2022 18:09

batconjurer reviewed Nov 1, 2022

View reviewed changes

batconjurer approved these changes Nov 1, 2022

View reviewed changes

sug0 reviewed Nov 2, 2022

View reviewed changes

james-chf mentioned this pull request Nov 2, 2022

Fix failing make test-wasm in eth-bridge-integration #699

Closed

james-chf merged commit 1a9a48e into eth-bridge-integration Nov 2, 2022

james-chf deleted the interop/ethbridge/merge-0.8.1 branch November 2, 2022 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `eth-bridge-integration` to v0.8.1 #704

Update `eth-bridge-integration` to v0.8.1 #704

james-chf commented Oct 27, 2022 •

edited

Loading

james-chf commented Oct 28, 2022

james-chf commented Oct 31, 2022

batconjurer Nov 1, 2022

sug0 Nov 2, 2022

tzemanovic Nov 2, 2022

batconjurer Nov 1, 2022

james-chf Nov 1, 2022

batconjurer Nov 1, 2022

batconjurer Nov 1, 2022

james-chf Nov 1, 2022

batconjurer Nov 1, 2022

james-chf Nov 1, 2022

sug0 Nov 2, 2022 •

edited

Loading

james-chf Nov 2, 2022

sug0 left a comment

sug0 Nov 2, 2022

james-chf Nov 2, 2022

sug0 Nov 2, 2022

james-chf Nov 2, 2022

sug0 Nov 2, 2022

sug0 Nov 2, 2022

sug0 Nov 2, 2022

sug0 Nov 2, 2022

sug0 Nov 2, 2022

sug0 Nov 2, 2022

sug0 Nov 2, 2022

	cli::AnomaClient::WithoutContext(cmd, global_args) => match cmd {
	cli::NamadaClient::WithoutContext(cmd, global_args) => match cmd {

Update eth-bridge-integration to v0.8.1 #704

Update eth-bridge-integration to v0.8.1 #704

Conversation

james-chf commented Oct 27, 2022 • edited Loading

james-chf commented Oct 28, 2022

james-chf commented Oct 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sug0 Nov 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sug0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Update `eth-bridge-integration` to v0.8.1 #704

Update `eth-bridge-integration` to v0.8.1 #704

james-chf commented Oct 27, 2022 •

edited

Loading

sug0 Nov 2, 2022 •

edited

Loading