Feat/send inter tx #27

seantking · 2021-03-23T15:37:58Z

Description
Adding the send command to the inter-tx module. We should now have a basic flow for registering an account and sending tokens with the registered interchain account.

I also updated the register command to use flags for source port and source channel as well as some general cleanup.

I have removed timeouts as user input and hardcoded a max int64 timeout period. (This should probably be lowered to something like 4 weeks).

I added a command in the Makefile to clear previous data & spin up both test chains & relayer in one command for convenience.

init: kill-dev install 
	@echo "Initializing both blockchains..."
	./network/init.sh
	@echo "Initializing relayer..." 
	./network/relayer/interchain-acc-config/rly.sh

How to run

# These address are defined in init.sh for development purposes
VAL_1=cosmos1mjk79fjjgpplak5wq838w0yd982gzkyfrk07am
VAL_2=cosmos17dtl0mjt3t77kpuhg2edqzjpszulwhgzuj9ljs

# Register an IBC Account on chain test-2 
icad tx intertx register --from val1 --source-port ibcaccount --source-channel channel-0 --chain-id test-1 --gas 90000 --home ./data/test-1 --node tcp://localhost:16657

# Get the address of interchain account
icad query intertx ibcaccount $VAL_1 ibcaccount channel-0 --node tcp://localhost:16657
# Output -> address: cosmos1pt6ar8lawmvvq5haxc3l3zhjfl04u56fs2ndh9

IBC_ACCOUNT=cosmos1pt6ar8lawmvvq5haxc3l3zhjfl04u56fs2ndh9

# Check the interchain account's balance on test-2 chain. It should be empty.
icad q bank balances $IBC_ACCOUNT --chain-id test-2

# Send some assets to $IBC_ACCOUNT.
icad tx bank send val2 $IBC_ACCOUNT 1000stake --chain-id test-2 --home ./data/test-2 --node tcp://localhost:26657

# Test sending assets on intetchain account via ibc.
icad tx intertx send cosmos-sdk $VAL_2 500stake --source-port ibcaccount --source-channel channel-0 --chain-id test-1 --gas 90000 --home ./data/test-1 --node tcp://localhost:16657 --from val1

# Wait until the relayer has relayed the packet

# Check if the balance has been changed.
icad q bank balances $IBC_ACCOUNT --chain-id test-2
icad q bank balances $VAL_2 --chain-id test-2

defihyung

Great work @seantking! The testing environment is smooth.
I left some nitpick comments below.

In regards of comments, what do you think about documenting go code as godoc suggests?

The convention is simple: to document a type, variable, constant, function, or even a package, write a regular comment directly preceding its declaration, with no intervening blank line. Godoc will then present that comment as text alongside the item it documents.......

defihyung · 2021-03-24T06:04:15Z

readme.md

+# Send some assets to $IBC_ACCOUNT.
+icad tx bank send val2 $IBC_ACCOUNT 1000stake --chain-id test-2 --home ./data/test-2 --node tcp://localhost:26657
+
+# Test sending assets on intetchain account via ibc.


little typo in this comment. intetchain should be interchain. Other than that, it is all good.

defihyung · 2021-03-24T07:47:58Z

x/inter-tx/keeper/msg_server.go

@@ -30,20 +31,29 @@ func (k msgServer) Register(
 	// check if an account is already registered
 	_, err = k.GetIBCAccount(ctx, msg.SourcePort, msg.SourceChannel, acc)
 	if err == nil {
-		return &types.MsgRegisterAccountResponse{}, fmt.Errorf("Interchain account is already registered for this account")
+		err = fmt.Errorf("Interchain account is already registered for this account")
+		return &types.MsgRegisterAccountResponse{}, sdkerrors.Wrap(err, err.Error())


Registering an IBC Account on chain test-2 with the same source-port and source-channel returns codespace undefined and code 1. How about handling this with sdkerrors.Wrap(types.ErrIAAccountNotExist, acc.String())?

Btw, do you know what IA stand for? interchain account?

All errors need to be registered otherwise the error message gets wiped downstream. The first error cannot begin with 1 either. Usually these errors are registered in errors.go

Updated this thanks

okwme · 2021-03-24T08:04:21Z

Makefile

 	rm -rf ./data
+	-@killall icad 2>/dev/null


why switch the order? is there a chance that the process starts writing to that location again and you need to delete all over again?

Ah, this line was throwing an error if there were no icad processes running. I'm silencing the error now with the - and the 2>/dev/null but this is the reason.

readme.md

okwme · 2021-03-24T08:17:15Z

readme.md

@@ -16,24 +16,40 @@ https://github.com/cosmos/relayer.git
 cd relayer
 make install



line 10 has old repo name

okwme · 2021-03-24T08:20:56Z

readme.md

-# Use relayer to link the chains
-make start-rly
+# Bootstrap two local chains & start the relayer with development data
+make init
 ```



Suggested change

Wait til you see the chains linked with the clients, connections and channels created.

Updated, although slightly differently. Good idea.

okwme · 2021-03-24T08:23:56Z

readme.md

+
+# Check if the balance has been changed.
+icad q bank balances $IBC_ACCOUNT --chain-id test-2
+icad q bank balances $VAL_2 --chain-id test-2
 ```



defihyung · 2021-03-24T09:33:15Z

Another feedback I had was to change the module name to github.com/cosmos/interchain-account since the repo is moved. This requires to update all proto files and imports inside both modules. It can be handled with another PR.

seantking · 2021-03-24T09:39:53Z

x/ibc-account/keeper/relay.go

-	// TODO: Add timeout height and timestamp
+	timeoutHeight := clienttypes.Height{
+		RevisionNumber: 2,
+		RevisionHeight: ^uint64(0),


@colin-axner did you say that I can set RevisionHeight to 0 to stop timeoutHeight from being used as a timeout? I was getting a relayer error when I did this.

I'm not sure what the best practice is here. @AdityaSripal maybe you have some thoughts also? 🙌

nvm updated

colin-axner

Great work! Did a quick pass and will look over any files I missed later this week. Left mostly code convention suggestions

x/ibc-account/keeper/relay.go

x/inter-tx/types/codec.go

x/inter-tx/types/msgs.go

proto/intertx/tx.proto

colin-axner · 2021-03-24T16:49:26Z

x/inter-tx/keeper/msg_server.go

@@ -30,20 +31,29 @@ func (k msgServer) Register(
 	// check if an account is already registered
 	_, err = k.GetIBCAccount(ctx, msg.SourcePort, msg.SourceChannel, acc)
 	if err == nil {
-		return &types.MsgRegisterAccountResponse{}, fmt.Errorf("Interchain account is already registered for this account")
+		err = fmt.Errorf("Interchain account is already registered for this account")
+		return &types.MsgRegisterAccountResponse{}, sdkerrors.Wrap(err, err.Error())


All errors need to be registered otherwise the error message gets wiped downstream. The first error cannot begin with 1 either. Usually these errors are registered in errors.go

proto/intertx/tx.proto

seantking · 2021-03-25T16:18:03Z

Great work @seantking! The testing environment is smooth.
I left some nitpick comments below.

In regards of comments, what do you think about documenting go code as godoc suggests?

The convention is simple: to document a type, variable, constant, function, or even a package, write a regular comment directly preceding its declaration, with no intervening blank line. Godoc will then present that comment as text alongside the item it documents.......

Good call re: Godoc. I have updated most of the functions in the inter-tx module to follow this standard. What do you think?

defihyung

LGTM

AdityaSripal

The timeout approach here looks good to me 👍

seantking added 7 commits March 23, 2021 12:00

feat(send cmd): adding send cmd scaffolding

8ef6786

feat(Makefile): adding init cmd

19fabc5

fix(remove ibc timeout): removing user input timeout

3551644

refactor(cli): updating send and register interface

af79d44

refactor(clean up): creating relay.go and doing some general clean up

79d9f64

refactor(error): updating grammar on error message

d4d20d9

refactor(error): throwing sdk error

f4eddb2

seantking force-pushed the feat/send-inter-tx branch 2 times, most recently from 42a0f18 to d68d5c4 Compare March 23, 2021 23:51

seantking marked this pull request as ready for review March 24, 2021 05:32

defihyung suggested changes Mar 24, 2021

View reviewed changes

okwme reviewed Mar 24, 2021

View reviewed changes

readme.md Show resolved Hide resolved

okwme approved these changes Mar 24, 2021

View reviewed changes

seantking commented Mar 24, 2021

View reviewed changes

refactor(read me): updating demo

e75aa11

seantking force-pushed the feat/send-inter-tx branch from d68d5c4 to e75aa11 Compare March 24, 2021 10:09

seantking mentioned this pull request Mar 24, 2021

Update ICS-27 Specification #22

Closed

seantking assigned AdityaSripal and colin-axner and unassigned AdityaSripal and colin-axner Mar 24, 2021

seantking requested review from AdityaSripal and colin-axner March 24, 2021 11:32

colin-axner reviewed Mar 24, 2021

View reviewed changes

refactor(comments): updating comments to align with godoc standard

718c26d

seantking force-pushed the feat/send-inter-tx branch from d342e1e to 2dd27d8 Compare March 25, 2021 16:37

refactor(clean up): general tidy and updating comments

1fdd98a

seantking force-pushed the feat/send-inter-tx branch from 2dd27d8 to 1fdd98a Compare March 25, 2021 16:43

defihyung approved these changes Mar 26, 2021

View reviewed changes

seantking merged commit 890f9dd into master Mar 26, 2021

seantking deleted the feat/send-inter-tx branch March 26, 2021 15:56

AdityaSripal reviewed Apr 14, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/send inter tx #27

Feat/send inter tx #27

seantking commented Mar 23, 2021 •

edited

Loading

defihyung left a comment •

edited

Loading

defihyung Mar 24, 2021 •

edited

Loading

seantking Mar 25, 2021

defihyung Mar 24, 2021

defihyung Mar 24, 2021

colin-axner Mar 24, 2021

seantking Mar 25, 2021

okwme Mar 24, 2021

seantking Mar 24, 2021 •

edited

Loading

okwme Mar 24, 2021

seantking Mar 25, 2021

okwme Mar 24, 2021

seantking Mar 25, 2021

okwme Mar 24, 2021

defihyung commented Mar 24, 2021

seantking Mar 24, 2021

seantking Mar 25, 2021

colin-axner left a comment

colin-axner Mar 24, 2021

seantking commented Mar 25, 2021

defihyung left a comment

AdityaSripal left a comment

		@@ -16,24 +16,40 @@ https://github.com/cosmos/relayer.git
		cd relayer
		make install


	Wait til you see the chains linked with the clients, connections and channels created.

Feat/send inter tx #27

Feat/send inter tx #27

Conversation

seantking commented Mar 23, 2021 • edited Loading

defihyung left a comment • edited Loading

Choose a reason for hiding this comment

defihyung Mar 24, 2021 • edited Loading

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

seantking Mar 24, 2021 • edited Loading

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

defihyung commented Mar 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seantking commented Mar 25, 2021

defihyung left a comment

Choose a reason for hiding this comment

AdityaSripal left a comment

Choose a reason for hiding this comment

seantking commented Mar 23, 2021 •

edited

Loading

defihyung left a comment •

edited

Loading

defihyung Mar 24, 2021 •

edited

Loading

seantking Mar 24, 2021 •

edited

Loading