refactor: rename RegisterCounterpartyAddress rpc to RegisterCounterpartyPayee #1513

damiannolan · 2022-06-08T16:48:35Z

Description

Renaming RegisterCounterpartyAddress rpc to RegisterCounterpartyPayee

closes: #XXXX

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov-commenter · 2022-06-09T12:34:11Z

Codecov Report

Merging #1513 (49ce2f6) into main (5e5e2cd) will decrease coverage by 0.00%.
The diff coverage is 96.22%.

@@            Coverage Diff             @@
##             main    #1513      +/-   ##
==========================================
- Coverage   80.27%   80.26%   -0.01%     
==========================================
  Files         166      166              
  Lines       12252    12254       +2     
==========================================
+ Hits         9835     9836       +1     
- Misses       1954     1955       +1     
  Partials      463      463

Impacted Files	Coverage Δ
modules/apps/29-fee/types/codec.go	`44.44% <50.00%> (+0.69%)`	⬆️
modules/apps/29-fee/ibc_middleware.go	`93.97% <100.00%> (ø)`
modules/apps/29-fee/keeper/genesis.go	`87.50% <100.00%> (ø)`
modules/apps/29-fee/keeper/grpc_query.go	`75.54% <100.00%> (ø)`
modules/apps/29-fee/keeper/keeper.go	`92.48% <100.00%> (ø)`
modules/apps/29-fee/keeper/msg_server.go	`96.00% <100.00%> (ø)`
modules/apps/29-fee/keeper/relay.go	`88.23% <100.00%> (ø)`
modules/apps/29-fee/types/genesis.go	`81.96% <100.00%> (ø)`
modules/apps/29-fee/types/keys.go	`97.14% <100.00%> (ø)`
modules/apps/29-fee/types/msgs.go	`90.75% <100.00%> (ø)`

charleenfei · 2022-06-09T15:16:58Z

proto/ibc/applications/fee/v1/tx.proto

+  string channel_id = 2 [(gogoproto.moretags) = "yaml:\"channel_id\""];
+  // the relayer address
+  string relayer_address = 3;
+  // the counterparty relayer address


nit: update the comment to counterparty payee

charleenfei · 2022-06-09T15:17:59Z

docs/ibc/proto-docs.md

 | `port_id` | [string](#string) |  | unique port identifier |
 | `channel_id` | [string](#string) |  | unique channel identifier |
+| `relayer_address` | [string](#string) |  | the relayer address |


nit: is address necessary in this naming if we don't have it for payee below?

charleenfei · 2022-06-09T15:18:36Z

proto/ibc/applications/fee/v1/tx.proto

-  string channel_id = 4 [(gogoproto.moretags) = "yaml:\"channel_id\""];
+  string channel_id = 2 [(gogoproto.moretags) = "yaml:\"channel_id\""];
+  // the relayer address
+  string relayer_address = 3;


same nit here re: either having relayer_address & counterparty_payee_address or just relayer & counterparty payee

Yeah actually, it's a fair point. I chatted with @AdityaSripal about this and we felt pretty indifferent about it.
If we are going to go with one or the other I think we should go with dropping the address suffix for consistency as counterparty_payee_address is a bit wordy. What do you think?

I think for purposes of consistency I'll go ahead and drop the address suffix and make the changes in RegisterPayee msgs also

charleenfei · 2022-06-09T15:21:39Z

modules/apps/29-fee/keeper/msg_server.go

+// RegisterCounterpartyPayee is called by the relayer on each channelEnd and allows them to specify their
+// counterparty address before relaying. This ensures they will be properly compensated for forward relaying since


Suggested change

// RegisterCounterpartyPayee is called by the relayer on each channelEnd and allows them to specify their

// counterparty address before relaying. This ensures they will be properly compensated for forward relaying since

// RegisterCounterpartyPayee is called by the relayer on each channelEnd and allows them to specify the

// counterparty payee address before relaying. This ensures they will be properly compensated for forward relaying since

I'll update these docs both here and in the proto files as they're replicated and push the changes 👍

charleenfei · 2022-06-09T15:22:14Z

modules/apps/29-fee/keeper/msg_server.go

+// RegisterCounterpartyPayee defines a rpc handler method for MsgRegisterCounterpartyPayee
+// RegisterCounterpartyPayee is called by the relayer on each channelEnd and allows them to specify their
+// counterparty address before relaying. This ensures they will be properly compensated for forward relaying since
+// destination chain must send back relayer's source address (counterparty address) in acknowledgement. This function


Suggested change

// destination chain must send back relayer's source address (counterparty address) in acknowledgement. This function

// destination chain must send back the counterparty payee in acknowledgement. This function

charleenfei · 2022-06-09T15:22:36Z

modules/apps/29-fee/keeper/msg_server.go

+// RegisterCounterpartyPayee is called by the relayer on each channelEnd and allows them to specify their
+// counterparty address before relaying. This ensures they will be properly compensated for forward relaying since
+// destination chain must send back relayer's source address (counterparty address) in acknowledgement. This function
+// may be called more than once by a relayer, in which case, the latest counterparty address is always used.


Suggested change

// may be called more than once by a relayer, in which case, the latest counterparty address is always used.

// may be called more than once, in which case, the latest counterparty payee address is always used.

modules/apps/29-fee/keeper/keeper.go

charleenfei · 2022-06-09T15:25:39Z

docs/ibc/proto-docs.md

@@ -1412,7 +1412,7 @@ Msg defines the ICS29 Msg service.
 | Method Name | Request Type | Response Type | Description | HTTP Verb | Endpoint |
 | ----------- | ------------ | ------------- | ------------| ------- | -------- |
 | `RegisterPayee` | [MsgRegisterPayee](#ibc.applications.fee.v1.MsgRegisterPayee) | [MsgRegisterPayeeResponse](#ibc.applications.fee.v1.MsgRegisterPayeeResponse) | RegisterPayee defines a rpc handler method for MsgRegisterPayee RegisterPayee is called by the relayer on each channelEnd and allows them to set an optional payee to which escrowed packet fees will be paid out. The payee should be registered on the source chain from which packets originate as this is where fee distribution takes place. This function may be called more than once by a relayer, in which case, the latest payee is always used. | |
-| `RegisterCounterpartyAddress` | [MsgRegisterCounterpartyAddress](#ibc.applications.fee.v1.MsgRegisterCounterpartyAddress) | [MsgRegisterCounterpartyAddressResponse](#ibc.applications.fee.v1.MsgRegisterCounterpartyAddressResponse) | RegisterCounterpartyAddress defines a rpc handler method for MsgRegisterCounterpartyAddress RegisterCounterpartyAddress is called by the relayer on each channelEnd and allows them to specify their counterparty address before relaying. This ensures they will be properly compensated for forward relaying since destination chain must send back relayer's source address (counterparty address) in acknowledgement. This function may be called more than once by a relayer, in which case, the latest counterparty address is always used. | |
+| `RegisterCounterpartyPayee` | [MsgRegisterCounterpartyPayee](#ibc.applications.fee.v1.MsgRegisterCounterpartyPayee) | [MsgRegisterCounterpartyPayeeResponse](#ibc.applications.fee.v1.MsgRegisterCounterpartyPayeeResponse) | RegisterCounterpartyPayee defines a rpc handler method for MsgRegisterCounterpartyPayee RegisterCounterpartyPayee is called by the relayer on each channelEnd and allows them to specify their counterparty address before relaying. This ensures they will be properly compensated for forward relaying since destination chain must send back relayer's source address (counterparty address) in acknowledgement. This function may be called more than once by a relayer, in which case, the latest counterparty address is always used. | |


isn't it that the counterparty address doesnt always have to be the relayer's source address?

Co-authored-by: Charly <[email protected]>

damiannolan · 2022-06-09T16:11:55Z

Thanks for the suggestions @charleenfei, I've updated to remove the address suffix on the msg fields and also updated both protodoc and godocs

charleenfei

nice work sham

crodriguezvega · 2022-06-13T07:26:21Z

docs/ibc/proto-docs.md

@@ -1412,7 +1412,7 @@ Msg defines the ICS29 Msg service.
 | Method Name | Request Type | Response Type | Description | HTTP Verb | Endpoint |
 | ----------- | ------------ | ------------- | ------------| ------- | -------- |
 | `RegisterPayee` | [MsgRegisterPayee](#ibc.applications.fee.v1.MsgRegisterPayee) | [MsgRegisterPayeeResponse](#ibc.applications.fee.v1.MsgRegisterPayeeResponse) | RegisterPayee defines a rpc handler method for MsgRegisterPayee RegisterPayee is called by the relayer on each channelEnd and allows them to set an optional payee to which escrowed packet fees will be paid out. The payee should be registered on the source chain from which packets originate as this is where fee distribution takes place. This function may be called more than once by a relayer, in which case, the latest payee is always used. | |
-| `RegisterCounterpartyAddress` | [MsgRegisterCounterpartyAddress](#ibc.applications.fee.v1.MsgRegisterCounterpartyAddress) | [MsgRegisterCounterpartyAddressResponse](#ibc.applications.fee.v1.MsgRegisterCounterpartyAddressResponse) | RegisterCounterpartyAddress defines a rpc handler method for MsgRegisterCounterpartyAddress RegisterCounterpartyAddress is called by the relayer on each channelEnd and allows them to specify their counterparty address before relaying. This ensures they will be properly compensated for forward relaying since destination chain must send back relayer's source address (counterparty address) in acknowledgement. This function may be called more than once by a relayer, in which case, the latest counterparty address is always used. | |
+| `RegisterCounterpartyPayee` | [MsgRegisterCounterpartyPayee](#ibc.applications.fee.v1.MsgRegisterCounterpartyPayee) | [MsgRegisterCounterpartyPayeeResponse](#ibc.applications.fee.v1.MsgRegisterCounterpartyPayeeResponse) | RegisterCounterpartyPayee defines a rpc handler method for MsgRegisterCounterpartyPayee RegisterCounterpartyPayee is called by the relayer on each channelEnd and allows them to specify the counterparty payee address before relaying. This ensures they will be properly compensated for forward relaying since the destination chain must include the registered counterparty payee address in the acknowledgement. This function may be called more than once by a relayer, in which case, the latest counterparty payee address is always used. | |


This ensures they will be properly compensated for forward relaying since the destination chain must include the registered counterparty payee address in the acknowledgement. This is not complete true with the introduction of the the MsgRegisterPayee, right? Because the payee that you register could get the fees even if the counterparty address is not registered. An operator could decide not to use the counterparty payee at all and only the payee, right?

I've updated must to may and included an additional note:

// A registered payee on the source chain will always override a registered counterparty payee.

Sorry, @damiannolan, I think my comment above is not correct. I just realized while I was writing this comment on the other PR. I think you still need to register the counterparty address, otherwise we wouldn't know on the source chain to which payee to pay out the fees. The counterparty address may or may not have a payee registered.

After the conversation this morning in the planning call, I think we need to update this comment again, right?

Yep, done! Feel free to suggest changes

crodriguezvega

Thank you, @damiannolan!

chatton

LGTM! Not major comments, just a small nit 💯

chatton · 2022-06-13T15:20:55Z

modules/apps/29-fee/client/cli/tx.go

+	cmd := &cobra.Command{
+		Use:     "register-counterparty-payee [port-id] [channel-id] [relayer] [counterparty-payee] ",
+		Short:   "Register a counterparty payee address on a given channel.",
+		Long:    strings.TrimSpace(`Register a counterparty payee address on a given channel.`),


[nit] unnecessary strings.TrimSpace call, this is just a regular string.

I left this in from existing code and I think there's a good few of them around. Maybe we could open a follow up PR to remove them if you want, some general housekeeping.

damiannolan added 4 commits June 8, 2022 18:17

renaming counterparty address rpc to counterparty payee

6e65707

updating counterparty payee cli

6ef0b44

Merge branch 'main' into damian/rename-counterparty-payee-rpc

cea4938

reorder cli cmd list

679e0ee

damiannolan marked this pull request as ready for review June 9, 2022 12:26

damiannolan requested review from AdityaSripal, seantking, charleenfei, colin-axner, crodriguezvega and chatton as code owners June 9, 2022 12:26

charleenfei reviewed Jun 9, 2022

View reviewed changes

damiannolan and others added 2 commits June 9, 2022 17:42

Apply suggestions from code review

6121ec0

Co-authored-by: Charly <[email protected]>

removing address suffix from msg fields, updating protodoc and godoc

ac2dd48

damiannolan added 2 commits June 9, 2022 18:13

updating cli args naming convention to align

473b113

Merge branch 'main' into damian/rename-counterparty-payee-rpc

7af30a6

This was referenced Jun 9, 2022

refactor: rename genesis type RegisteredRelayer to RegisteredCounterpartyPayee #1514

Merged

refactor: rename CounterpartyAddress query to CounterpartyPayee #1522

Merged

charleenfei approved these changes Jun 10, 2022

View reviewed changes

updating keys to reflect naming changes

49ce2f6

crodriguezvega reviewed Jun 13, 2022

View reviewed changes

updating docs

d8511e4

damiannolan requested a review from fedekunze as a code owner June 13, 2022 08:04

updating protodocs and godocs

88f01c2

crodriguezvega approved these changes Jun 13, 2022

View reviewed changes

Merge branch 'main' into damian/rename-counterparty-payee-rpc

e865222

chatton approved these changes Jun 13, 2022

View reviewed changes

Merge branch 'main' into damian/rename-counterparty-payee-rpc

bdb821a

crodriguezvega merged commit dab07f9 into main Jun 14, 2022

crodriguezvega deleted the damian/rename-counterparty-payee-rpc branch June 14, 2022 10:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: rename RegisterCounterpartyAddress rpc to RegisterCounterpartyPayee #1513

refactor: rename RegisterCounterpartyAddress rpc to RegisterCounterpartyPayee #1513

damiannolan commented Jun 8, 2022 •

edited

Loading

codecov-commenter commented Jun 9, 2022 •

edited

Loading

charleenfei Jun 9, 2022

damiannolan Jun 9, 2022

charleenfei Jun 9, 2022

charleenfei Jun 9, 2022

damiannolan Jun 9, 2022

damiannolan Jun 9, 2022

charleenfei Jun 9, 2022

damiannolan Jun 9, 2022

charleenfei Jun 9, 2022

charleenfei Jun 9, 2022

charleenfei Jun 9, 2022

damiannolan commented Jun 9, 2022

charleenfei left a comment

crodriguezvega Jun 13, 2022

damiannolan Jun 13, 2022

crodriguezvega Jun 13, 2022

crodriguezvega Jun 13, 2022

damiannolan Jun 13, 2022 •

edited

Loading

crodriguezvega left a comment •

edited

Loading

chatton left a comment

chatton Jun 13, 2022

damiannolan Jun 13, 2022

		// RegisterCounterpartyPayee is called by the relayer on each channelEnd and allows them to specify their
		// counterparty address before relaying. This ensures they will be properly compensated for forward relaying since

	// destination chain must send back relayer's source address (counterparty address) in acknowledgement. This function
	// destination chain must send back the counterparty payee in acknowledgement. This function

	// may be called more than once by a relayer, in which case, the latest counterparty address is always used.
	// may be called more than once, in which case, the latest counterparty payee address is always used.

refactor: rename RegisterCounterpartyAddress rpc to RegisterCounterpartyPayee #1513

refactor: rename RegisterCounterpartyAddress rpc to RegisterCounterpartyPayee #1513

Conversation

damiannolan commented Jun 8, 2022 • edited Loading

Description

codecov-commenter commented Jun 9, 2022 • edited Loading

Codecov Report

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

damiannolan commented Jun 9, 2022

charleenfei 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

damiannolan Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

crodriguezvega left a comment • edited Loading

Choose a reason for hiding this comment

chatton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damiannolan commented Jun 8, 2022 •

edited

Loading

codecov-commenter commented Jun 9, 2022 •

edited

Loading

damiannolan Jun 13, 2022 •

edited

Loading

crodriguezvega left a comment •

edited

Loading