Review by GregTheGreek · Pull Request #1 · GregTheGreek/ChainBridge

GregTheGreek · 2020-04-11T15:02:42Z

Changes

Closes: #?

- Adds basic CLI and main package - Adds PR template file

- Adds Connection, Listener, and Pusher interfaces and started Ethereum & Centrifuge implementations - Adds general Chain type and Core to handle the orchestration of them

* implement ethereum connection using ethclient and rpcclient * implement SendTx (currently fails cause no signing :( ) * implement event subscriptions * added event types (just strings for now, we will need to determine what events we actually want)

- Adds api object to centrifuge connection.

- Fixes linting issues - Skip tests

* makefile, test script & github CI configured * Updates golangci config * changed lint timeout * removed verbosity; formatted project; fixed lint error

- Introduces `ChainConfig` - Able to load config from toml - Enable config and verbosity flags, adds keystore path flag

* fix go.mod

* add keypair to eth connection * implement signing of tx in SubmitTx for ethereum * to sign a tx, the function takes a Signer which is created from a geth ChainConfig. we need to add which eth chain config to use to the config, see todo in chains/ethereum/ethereum.go

* adds account subcommand with generate, list, and import functionality. saves keys by default in ./keys/keystore. currently, only creates secp256k1 keys and saves them in an encrypted file.

- basic router to forward messages to destination writer - adds router to startup - moves interfaces to `chain`

- Adds ability to subscribe to ethereum events - Events provided by config - Adds `Start`, `Stop` to listener and writer - Moves event types to `ethereum`

* add contracts * cleanup code * remove multisig code * remove admin * add validator * remove null from vote * add prefix Deposit to vote and proposals * address changes * fix final change * rename contacts dir * truffle * remove embark * add vlaidator count * test work * add validator proposal tests * add node workflow * clean up node yaml ; * cd to correct dir * rename to receiver * minor changes * add basic deposit tests * remove pacakge * seperate tests * stuff * add more tests * rename contrat tests * refactor * add theshold tests * add edge case * refactor tests to individual files * update Makefile * address comments * add coments * fix test

* add contracts * cleanup code * remove multisig code * remove admin * add validator * remove null from vote * add prefix Deposit to vote and proposals * address changes * fix final change * rename contacts dir * truffle * remove embark * add vlaidator count * test work * add validator proposal tests * add node workflow * clean up node yaml ; * cd to correct dir * rename to receiver * minor changes * add basic deposit tests * remove pacakge * seperate tests * stuff * add more tests * rename contrat tests * refactor * add theshold tests * add edge case * refactor tests to individual files * update Makefile * address comments * add coments * fix test * add startup scripts for ethereum * add additional cli args * add cli * fix truffle test * remove log * fix scripts

added centrifuge's asset contract and test that calls their store function. update ResolveMessage in eth package and message type. also added some helper functions for getting account nonce and function IDs.

Renames Home/Away to Receiver/Emitter

- Changes command `account` to `accounts` (sorry, nit picky) - Uses full provided path to keystore (doesn't append `/keys`) - Removes global flags from sub-command - Uses `Global*` for looking up global flags - Remove logs for errors that are also returned

- Remove usage of `sh` - Fixes `make run` - Adds `make install` - Adds readme instructions (WIP) - Change binary to `chainbridge` - Keep bridge alive until killed (ctrl-c) - Properly pass router to listener

- Reduces functionality of keystore to bare necessities - No need for in memory KS, just needs to load specified keypairs - Adds mock keystore for testing - Use `NewTestKeystore()` - Address "ethereum" returns default ethereum keypair, can be extended to others - Use table of chains in toml config - Gives us an array instead of map, easier to generalize - Introduces `RawChainConfig` to provide an interface for `.toml` config - Introduces `ethereum.Config` to hold the params for all the ethereum components - Updates readme

- Basic connection using GSRPC lib - Listener with naive logic for handling NFT events - Scripts for testing - `make start_cent` which fetches, builds, and runs centrifuge-chain - `make cent_asset_tx` which submits a random hash that is emitted as the NFT hash

- Adds ed25519 and sr25519 keys - Enables `account` flags and handlers

* add md for centrifuge start * move into chain doc * add link to readme

* stuff * mods * changs * further changes * organise * update erc20 resolver * broke things * whitespace * fix tests * fix tests * rename contracts * address comments * address commnets * updated comments * update fn name

- Cleanup listener - Adds hash transfer test - Switch CI to chainbridge-substrate-chain - Clean up a bit in preparation for deployment

* Importing Geth Keys

* refctor deploy command * lint * add writer tests * update hash * fix method test * more proposal testing and voting * debuggin execute deposit * vote yes * fix linter * convert to helper function * test helper * refactor * update per comments * refactor based on suggestions * fix test * finally fixed the nonce bug * add mock * cleaned up * add basic listener test * lint and cleanup * add license * move to test * change address * fix

* Adding Logging

* fixing up * traced back to callopts nil pointer dereference * now it's message mismatches * off by one, thanks david * fixed event topic index for nonce in depositerc20 * remove adding one to the nonce in createDepositProposal * commented out writer vote and deposit proposal tests as they're to be changed shortly * cleaned up, tests passing * update to latest master commit from chainbridge-solidity * fixed merge issues * commented out unusused function + removed nonce workaround

- Adds go bindings, removes need for everyone to run `make setup-contracts` - Updates readme

Checking for Unrecognized Opts

- Updates message format to match discussions - Updates to latest contracts - Eth writer now watches for ProposalFinazlied event - Removes eth usage of messaging for internal purposes (signalling events to writer)

* removes outdated commands

- Updates bindings to latest - Uses new token id format

* Checking chainId on startup

- Update to latest contracts (hashing of data, data formatting) - Fixes generic handler support - Adds `startBlock` opt for listener

- Adds susbtrate hash -> eth test - Adds er20 -> susbtrate test - Adds CI e2e build

- Rebuilt bindings - Remove bytecode comparison - Add checks for other contracts

- Extends existing E2E to perfor multiple txs (starts ChainSafe#304) - Updates to the latest contract event - Implements susbtrate->eth test, skipped until ChainSafe/chainbridge-substrate#28 is closed

* Moving CLI elements

* Adding CodeUpdated Event

GregTheGreek · 2020-04-11T15:10:23Z

chains/ethereum/chain.go

+	if err != nil {
+		return nil, err
+	}
+


Why don't we initialize the generic contract?

Updated in ChainSafe#320 (ChainSafe#323)

GregTheGreek · 2020-04-11T15:17:44Z

chains/ethereum/connection.go

+}
+
+func NewConnection(cfg *Config, kp *secp256k1.Keypair, log log15.Logger) *Connection {
+	signer := ethtypes.HomesteadSigner{}


This will need to be configurable.

Done in ChainSafe#327

GregTheGreek · 2020-04-11T15:27:20Z

chains/ethereum/connection.go

+
+// SubmitTx submits a transaction to the chain
+// It assumes the input data is an ethtypes.Transaction, marshalled as JSON
+func (c *Connection) SubmitTx(data []byte) error {


I was thinking, could we capture the Nonce at this stage?
Furthermore, we might benefit from a queue, where we assign the Nonce as we fire them off.

To be removed, we don't ever use SubmitTx ChainSafe#323

GregTheGreek · 2020-04-11T15:28:44Z

chains/ethereum/deploy.go

+		return nil, err
+	}
+
+	deployedContracts := DeployedContracts{bridgeAddr, relayerAddr, erc20HandlerAddr, erc721HandlerAddr, centrifugeHandlerAddr}


NIT: No need to assign you can just return

GregTheGreek · 2020-04-11T15:33:39Z

chains/ethereum/listener.go

+// RegisterEventHandler creates a subscription for the provided event on the bridge bridgeContract.
+// Handler will be called for every instance of event.
+func (l *Listener) RegisterEventHandler(subscription EventSig, handler evtHandlerFn) error {
+	evt := subscription


This seems unnecessary

GregTheGreek · 2020-04-11T15:58:44Z

chains/ethereum/writer.go

+	case msg.NonFungibleTransfer:
+		panic("not yet implemented")
+	case msg.GenericTransfer:
+		return w.createGenericDepositProposal(m)


I dont see the contract being set, I'm confused at the pattern being followed for handling generic transfers?

All writes are to the bridge contract, updated in ChainSafe#320

GregTheGreek · 2020-04-11T15:59:17Z

chains/ethereum/writer.go

+func hash(data []byte) [32]byte {
+	return crypto.Keccak256Hash(data)
+}
+
+func u32toBigInt(n uint32) *big.Int {
+	return big.NewInt(int64(n))
+}
+
+func byteSliceTo32Bytes(in []byte) [32]byte { //nolint:deadcode,unused
+	out := [32]byte{}
+	// Note: this is safe as copy uses the min length of the two slices
+	copy(out[:], in)
+	return out
+}


It might make sense to push these out to a helper file

Removed in ChainSafe#320

GregTheGreek · 2020-04-11T16:00:06Z

chains/ethereum/writer_methods.go

+const ExecuteDepositMethod = "executeDepositProposal"
+
+func constructErc20ProposalData(amount, tokenId, recipient []byte) []byte {
+	var data []byte


Link to the solidity code where this is defined might be helpfull

David Ansermino and others added 30 commits November 28, 2019 17:54

Adds basic cli

c1c295f

Udpates readme

bda2fc0

Adds pr template

794c6a3

Merge pull request ChainSafe#22 from ChainSafe/david/setup

2ab5f87

- Adds basic CLI and main package - Adds PR template file

Basic interfaces (ChainSafe#26)

1662c5a

- Adds Connection, Listener, and Pusher interfaces and started Ethereum & Centrifuge implementations - Adds general Chain type and Core to handle the orchestration of them

Centrifuge connection (ChainSafe#30)

7c71ff1

- Adds api object to centrifuge connection.

Fix master (ChainSafe#32)

7583d91

- Fixes linting issues - Skip tests

makefile, test script & github CI configured (ChainSafe#31)

8dd128d

* makefile, test script & github CI configured * Updates golangci config * changed lint timeout * removed verbosity; formatted project; fixed lint error

Keystore & Config file stuff (ChainSafe#33)

e585fae

- Introduces `ChainConfig` - Able to load config from toml - Enable config and verbosity flags, adds keystore path flag

fix go.mod (ChainSafe#36)

e29b325

* fix go.mod

fixed make build command (ChainSafe#38)

c17da77

account subcommand for cli to manage keystore (ChainSafe#42)

6d97eaa

* adds account subcommand with generate, list, and import functionality. saves keys by default in ./keys/keystore. currently, only creates secp256k1 keys and saves them in an encrypted file.

Basic Router (ChainSafe#45)

92873d6

- basic router to forward messages to destination writer - adds router to startup - moves interfaces to `chain`

Ethereum Event Subscription (ChainSafe#43)

aabcdaf

- Adds ability to subscribe to ethereum events - Events provided by config - Adds `Start`, `Stop` to listener and writer - Moves event types to `ethereum`

update make build (ChainSafe#46)

51f683c

centrifuge contract, writing to ethereum, eth-to-eth (ChainSafe#49)

960f8a8

added centrifuge's asset contract and test that calls their store function. update ResolveMessage in eth package and message type. also added some helper functions for getting account nonce and function IDs.

Rename to Emitter/Recevier (ChainSafe#53)

eeed5c8

Renames Home/Away to Receiver/Emitter

use correct keys from mnemonic (ChainSafe#54)

be69742

Readme Updates & Script Fixes (ChainSafe#52)

1e9b3d2

- Remove usage of `sh` - Fixes `make run` - Adds `make install` - Adds readme instructions (WIP) - Change binary to `chainbridge` - Keep bridge alive until killed (ctrl-c) - Properly pass router to listener

Cleanups instructions (ChainSafe#57)

a5820bd

Additional Key Types (ChainSafe#60)

9bbb063

- Adds ed25519 and sr25519 keys - Enables `account` flags and handlers

Centrifuge start doc (ChainSafe#63)

1788b9c

* add md for centrifuge start * move into chain doc * add link to readme

Contracts (ChainSafe#61)

1eae9b8

* stuff * mods * changs * further changes * organise * update erc20 resolver * broke things * whitespace * fix tests * fix tests * rename contracts * address comments * address commnets * updated comments * update fn name

David Ansermino and others added 21 commits March 30, 2020 07:48

Adds implementation section to readme (ChainSafe#280)

c513c22

Substrate listener tests (ChainSafe#274)

491fb07

- Cleanup listener - Adds hash transfer test - Switch CI to chainbridge-substrate-chain - Clean up a bit in preparation for deployment

Importing Geth Keystore Files (ChainSafe#262)

3963ae2

* Importing Geth Keys

Updating Logging (ChainSafe#273)

6bacb1c

* Adding Logging

Add bindings (ChainSafe#285)

0b84b88

- Adds go bindings, removes need for everyone to run `make setup-contracts` - Updates readme

Checking for Unrecongized Opts (ChainSafe#283)

b1cb618

Checking for Unrecognized Opts

Message Updates & Latest Contracts (ChainSafe#288)

d49adac

- Updates message format to match discussions - Updates to latest contracts - Eth writer now watches for ProposalFinazlied event - Removes eth usage of messaging for internal purposes (signalling events to writer)

Update readme and makefile (ChainSafe#286)

29f9574

* removes outdated commands

Update contracts (ChainSafe#291)

39d6786

- Updates bindings to latest - Uses new token id format

Checking chainId on startup (ChainSafe#289)

32d3dc2

* Checking chainId on startup

update readme (ChainSafe#294)

152449e

Generic deposit and latest contracts (ChainSafe#295)

7719e3f

- Update to latest contracts (hashing of data, data formatting) - Fixes generic handler support - Adds `startBlock` opt for listener

E2E Tests (ChainSafe#292)

01863d2

- Adds susbtrate hash -> eth test - Adds er20 -> susbtrate test - Adds CI e2e build

Latest solidity (ChainSafe#299)

43831ef

Update diagrams (ChainSafe#298)

a0c1473

Fix binding check (ChainSafe#311)

7990dd1

- Rebuilt bindings - Remove bytecode comparison - Add checks for other contracts

Extend E2E, Update contracts (ChainSafe#315)

3f49311

- Extends existing E2E to perfor multiple txs (starts ChainSafe#304) - Updates to the latest contract event - Implements susbtrate->eth test, skipped until ChainSafe/chainbridge-substrate#28 is closed

Remove Deploy from CLI (ChainSafe#300)

de20777

* Moving CLI elements

Adding CodeUpdated Event (ChainSafe#316)

d8356b9

* Adding CodeUpdated Event

GregTheGreek commented Apr 11, 2020

View reviewed changes

GregTheGreek merged commit d8356b9 into review1 Apr 15, 2020

Conversation

GregTheGreek commented Apr 11, 2020

Changes

Closes: #?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants