refactor: splitting omni-connector into multiple smaller connectors #48

frolvanya · 2024-11-14T15:32:27Z

I have a few more changes locally, but I decided to create at least a draft PR to get an initial review

TODO:

rename omni-connector to evm-connector
push solana changes
make an abstraction for all calls and name it omni-connector

Probably last one is a task for another PR, since it's getting too big already and it would require to revamp whole bridge-cli as well. My current idea for abstraction is looking like this:

// omni-connector.rs

struct OmniConnector {
    eth_connector: Option<EvmConnector>,
    base_connector: Option<EvmConnector>,
    arb_connector: Option<EvmConnector>,
    sol_connector: Option<SolConnector>,
}

impl OmniConnector {
    fn init_transfer(args: InitTransferArgs) -> Result<TxHash> {
        match args.recipient.chain_kind() {
            ChainKind::Eth => eth_connector.unwrap().init_transfer(args),
            ChainKind::Base => base_connector.unwrap().init_transfer(args),
            ChainKind::Arb => arb_connector.unwrap().init_transfer(args),
            ChainKind::Sol => sol_connector.unwrap().init_transfer(args),
            _ => unimplemented!()
        }
    }
}

Questions:

near-connector sounds strange since it doesn't really connecting anything, just calls some rpc and stores information like private key and endpoint. Do you have any suggestions for a better naming
We have a lot of connectors already. Can we split them into legacy and new ones? Like it's a bit confusing to have an eth-connector and nep141-connector at the same time with near-connector and evm-connector

Since we can directly set path for a lib file, then there's no need to create a lib file just to import a module

kiseln · 2024-11-18T14:05:52Z

I agree that the word "connector" might not even be suitable anymore. We can think of a different name and rename at some point

Made an abstraction for init&fin transfer as well

bridge-cli/Cargo.toml

bridge-cli/src/main.rs

bridge-cli/src/nep141_connector_command.rs

olga24912 · 2024-12-05T10:30:45Z

bridge-cli/src/omni_connector_command.rs

        #[clap(short, long)]
-        source_chain_id: u8,
+        account_id: String,


The names are unclear. Could you add comments here explaining what they mean? Also, maybe use AccountId instead of String to make it clear that this refers specifically to a NEAR account.

Could you add comments here explaining what they mean?

I believe that's a job for another PR

bridge-cli/src/omni_connector_command.rs

bridge-sdk/connectors/omni-connector/src/omni_connector.rs

bridge-sdk/connectors/near-connector/src/near_connector.rs

frolvanya · 2024-12-05T15:03:15Z

@olga24912 Almost all issues were already fixed in #54

* feat: finished abstraction * feat: included solana into omni abstraction * feat: made solana's fields optional This can be useful if user won't use full potential of bridge-client, but only some methods which don't require all fields like wormhole or keypair * chore: provided a bit more consistency between different bridge clients * chore: addressed some clippy warnings * chore: updated some crates

olga24912 · 2024-12-06T09:53:53Z

bridge-cli/src/omni_connector_command.rs

+
+    EvmDeployToken {
+        #[clap(short, long)]
+        source_chain_id: u8,


Could you add a help explaining what the arguments mean? I’m already getting confused. Also, can we combine the commands with Solana equivalent and handle what needs to be done in the function?

Arguments are used during the cli input. Example:

cr -- testnet omni-connector bind-token-wormhole --source-chain-id '<source-chain-id>' --vaa '<hex-vaa>' --config-file config.json

Source chain id can be found here:

0 => Eth 1 => Near 2 => Sol 3 => Arb 4 => Base

I don't really understand what do you mean by combining the code. I'd rather finish working on cli later, it's not a highest priority right now. I just made sure to refactor code, I wasn't improving it on the cli's part

I mean that we need to add help for a command like this:

cargo run -- testnet omni-connector wormhole-bind-token --help

To explain meaning of each arg.

This should be really simple to do—probably just adding a description or something. I’m sure it can be easily googled.

olga24912 · 2024-12-06T10:02:46Z

bridge-cli/src/omni_connector_command.rs

+        #[clap(short, long)]
+        source_chain_id: u8,
+        #[clap(short, long)]
+        vaa: String,


It would be great to have the option to use a non-Wormhole prover here. This could be a separate task in a new PR. Could you create an issue for this?

olga24912 · 2024-12-06T10:26:38Z

bridge-sdk/bridge-clients/near-bridge-client/src/near_bridge_client.rs

+            })
+            .to_string()
+            .into_bytes(),
+            300_000_000_000_000,


make it const

Like this a0ae2b8?

olga24912 · 2024-12-06T10:33:28Z

bridge-sdk/connectors/omni-connector/src/omni_connector.rs

+            146, 2, 127, 83, 201, 149, 246, 138, 221, 29, 111, 186, 167, 150, 196, 102, 219, 89,
+            69, 115, 114, 185, 116, 6, 233, 154, 114, 222, 142, 167, 206, 157, 39, 177, 221, 224,
+            86, 146, 61, 226, 206, 55, 2, 119, 12,
+        ];


It would be great to create a function to calculate this. This can be done in a separate PR, as near_bridge_account might be different for testing purposes.

Yeah, I asked @kiseln about this already. We will do it later #56

frolvanya · 2024-12-07T00:09:47Z

@olga24912 all issues regarding bridge-cli will be fixed later. I've created an issue for this #57

refactor: scratch implementation of refactored version of sdk

80654de

frolvanya requested a review from kiseln November 14, 2024 15:32

refactor: made solana-connector rely on near-connector

290f637

frolvanya marked this pull request as ready for review November 15, 2024 16:50

chore: removed useless lib.rs files

23dcf5a

Since we can directly set path for a lib file, then there's no need to create a lib file just to import a module

frolvanya and others added 7 commits November 20, 2024 15:24

fix: manually synced with main branch

a806975

Merge branch 'main' into refactor/connectors

63e4737

fix: resolved issues after merge

3b3ae3b

chore: scratch implementation of bridge-clients

768ce63

Implement a few near<->evm operations (#52)

ee230e7

feat: added wormhole support

f4945f4

Made an abstraction for init&fin transfer as well

chore: bumped spl crates version

73be26e

frolvanya force-pushed the refactor/connectors branch from af0a8d9 to 73be26e Compare November 28, 2024 02:28

Ivan Frolov added 4 commits November 27, 2024 22:08

feat: support for multiple evm chains

c229e7b

chore: added abstract method for calling sign_transfer on NEAR chain

1d56481

chore: added abstract method for calling claim_fee on NEAR chain

24ed394

chore: added abstract method for calling sign_claim_fee on NEAR chain

3552776

frolvanya force-pushed the refactor/connectors branch from 820ad2f to 3552776 Compare November 28, 2024 04:51

feat: synced with latest omni-types

367043d

frolvanya force-pushed the refactor/connectors branch from a6b98eb to 367043d Compare November 28, 2024 07:01

frolvanya added 3 commits November 29, 2024 13:51

feat: added get_required_storage_deposit method on NEAR chain

cb893f9

fix: static analysis

d4393e7

feat: added get_token_id method on NEAR chain

d0e368d

frolvanya force-pushed the refactor/connectors branch from 113ff69 to d0e368d Compare November 30, 2024 02:03

feat: support multiple OmniAddress variants

1c6e332

karim-en requested a review from olga24912 December 2, 2024 22:58

feat: added get_native_token_id method on NEAR chain

8b7983b

frolvanya requested a review from karim-en December 4, 2024 20:02

olga24912 requested changes Dec 5, 2024

View reviewed changes

frolvanya requested a review from olga24912 December 5, 2024 15:52

chore: decreased attached gas

2585f2e

kiseln approved these changes Dec 5, 2024

View reviewed changes

olga24912 requested changes Dec 6, 2024

View reviewed changes

chore: moved attached gas/deposit to consts

a0ae2b8

frolvanya requested a review from olga24912 December 7, 2024 00:05

frolvanya mentioned this pull request Dec 7, 2024

[OmniBridge]: fix known bridge-cli issues #57

Closed

3 tasks

olga24912 approved these changes Dec 9, 2024

View reviewed changes

kiseln approved these changes Dec 9, 2024

View reviewed changes

frolvanya merged commit a98ba59 into main Dec 9, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: splitting omni-connector into multiple smaller connectors #48

refactor: splitting omni-connector into multiple smaller connectors #48

frolvanya commented Nov 14, 2024 •

edited

Loading

kiseln commented Nov 18, 2024

olga24912 Dec 5, 2024

frolvanya Dec 7, 2024

frolvanya commented Dec 5, 2024

olga24912 Dec 6, 2024

frolvanya Dec 6, 2024

olga24912 Dec 9, 2024

olga24912 Dec 9, 2024

olga24912 Dec 6, 2024

frolvanya Dec 6, 2024

olga24912 Dec 6, 2024

frolvanya Dec 7, 2024

olga24912 Dec 6, 2024

frolvanya Dec 6, 2024

frolvanya commented Dec 7, 2024

refactor: splitting omni-connector into multiple smaller connectors #48

refactor: splitting omni-connector into multiple smaller connectors #48

Conversation

frolvanya commented Nov 14, 2024 • edited Loading

kiseln commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frolvanya commented Dec 5, 2024

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

frolvanya commented Dec 7, 2024

frolvanya commented Nov 14, 2024 •

edited

Loading