feat: adding RegisterPayee rpc to ics29 fee middleware #1491

damiannolan · 2022-06-06T10:37:28Z

Description

Adding new rpc endpoint RegisterPayee to ics29 fee middleware

NOTE: this is one of many PRs to complete the feature work referenced in: #1477

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

…ion. Store funcs, keys..

codecov-commenter · 2022-06-06T10:46:59Z

Codecov Report

Merging #1491 (3b2ccda) into main (46f5eee) will increase coverage by 0.05%.
The diff coverage is 97.05%.

@@            Coverage Diff             @@
##             main    #1491      +/-   ##
==========================================
+ Coverage   80.36%   80.42%   +0.05%     
==========================================
  Files         166      166              
  Lines       12105    12176      +71     
==========================================
+ Hits         9728     9792      +64     
- Misses       1920     1926       +6     
- Partials      457      458       +1

Impacted Files	Coverage Δ
modules/apps/29-fee/types/msgs.go	`90.75% <93.54%> (+0.98%)`	⬆️
modules/apps/29-fee/keeper/keeper.go	`92.82% <100.00%> (+0.42%)`	⬆️
modules/apps/29-fee/keeper/msg_server.go	`96.00% <100.00%> (+0.83%)`	⬆️
modules/apps/29-fee/types/keys.go	`97.14% <100.00%> (+0.47%)`	⬆️
...les/apps/27-interchain-accounts/host/ibc_module.go	`100.00% <0.00%> (ø)`
.../apps/27-interchain-accounts/host/keeper/events.go	`0.00% <0.00%> (ø)`

crodriguezvega

Awesome! Once we merge these PRs for the distribution address, we can cut a new beta tag and share with crypto.com.

crodriguezvega · 2022-06-06T20:32:35Z

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

@@ -241,6 +241,24 @@ func (k Keeper) DeleteForwardRelayerAddress(ctx sdk.Context, packetID channeltyp
 	store.Delete(key)
 }

+// GetDistributionAddress retrieves the fee distribution address stored in state given the provided channel identifier and relayer address


This is a nit, but should we add these functions next to the ones to get and set the counterparty address?

Yep, done! I've moved to above counterparty getter/setter funcs. And also re-ordered the rpc definitions in proto so RegisterPayee comes before RegisterCounterpartyPayee

crodriguezvega · 2022-06-06T20:33:29Z

modules/apps/29-fee/types/errors.go

@@ -16,4 +16,5 @@ var (
 	ErrFeeNotEnabled                 = sdkerrors.Register(ModuleName, 9, "fee module is not enabled for this channel. If this error occurs after channel setup, fee module may not be enabled")
 	ErrRelayerNotFoundForAsyncAck    = sdkerrors.Register(ModuleName, 10, "relayer address must be stored for async WriteAcknowledgement")
 	ErrFeeModuleLocked               = sdkerrors.Register(ModuleName, 11, "the fee module is currently locked, a severe bug has been detected")
+	ErrDistributionAddressEmpty      = sdkerrors.Register(ModuleName, 12, "distribution address must not be empty")


Also a nit: since we haven't released fee yet, should we put this error next to ErrCounterpartyAddressEmpty?

Removed this error as its obsolete now that we check the payee field directly using sdk.AccAddressFromBech32 in ValidateBasic.

crodriguezvega · 2022-06-06T20:42:17Z

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

+  // the fee distribution address
+  string distribution_address = 2 [(gogoproto.moretags) = "yaml:\"distribution_address\""];
+  // unique port identifier
+  string port_id = 3 [(gogoproto.moretags) = "yaml:\"port_id\""];


Also a nit: should we change the order of the parameters to port_id, channel_id, address, distribution_address? Also for the register counterparty address command. I think in other messages the port and channel are the first parameters...

damiannolan · 2022-06-07T12:20:59Z

modules/apps/29-fee/types/msgs.go

+	_, err := sdk.AccAddressFromBech32(msg.Address)
+	if err != nil {
+		return sdkerrors.Wrap(err, "failed to convert msg.Address into sdk.AccAddress")
+	}


I need to add the same for msg.DistributionAddress

its also worth checking here that msg.Address != msg.DistributionAddress since that is a waste of a message/mapping

AdityaSripal

We should also check that addresses in ValidateBasic are different, everything else looks good!

AdityaSripal · 2022-06-07T12:26:20Z

modules/apps/29-fee/types/msgs.go

+	_, err := sdk.AccAddressFromBech32(msg.Address)
+	if err != nil {
+		return sdkerrors.Wrap(err, "failed to convert msg.Address into sdk.AccAddress")
+	}


its also worth checking here that msg.Address != msg.DistributionAddress since that is a waste of a message/mapping

AdityaSripal · 2022-06-07T12:27:16Z

modules/apps/29-fee/types/msgs.go

+
+// GetSigners implements sdk.Msg
+func (msg MsgRegisterDistributionAddress) GetSigners() []sdk.AccAddress {
+	signer, err := sdk.AccAddressFromBech32(msg.Address)


Yup, this is already the correct and sufficient authentication

AdityaSripal · 2022-06-07T12:29:22Z

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

+  option (gogoproto.goproto_getters) = false;
+
+  // the relayer address
+  string address = 1;


rename to relayer_address?

Should we also rename it like this in the register counterparty message?

Yep, will do this in a follow up PR tomorrow to reduce the number of changes! We will rename to RegisterCounterpartyPayee and align the same field names..etc 👍

AdityaSripal

Nice work!

damiannolan added 3 commits June 3, 2022 16:20

[WIP] adding RegisterDistributionAddress rpc endpoint and implementat…

a0291bd

…ion. Store funcs, keys..

updating tests

55e69b1

adding cli for register distribution address rpc

e09d0d3

damiannolan requested review from AdityaSripal, seantking, charleenfei, colin-axner, crodriguezvega and chatton as code owners June 6, 2022 10:37

This was referenced Jun 6, 2022

feat: adding RegisteredPayees to ics29 genesis state #1492

Merged

feat: adding Payee grpc query and CLI #1493

Merged

damiannolan mentioned this pull request Jun 6, 2022

Allow Relayers to overwrite payee address #1477

Closed

crodriguezvega approved these changes Jun 6, 2022

View reviewed changes

crodriguezvega reviewed Jun 6, 2022

View reviewed changes

damiannolan commented Jun 7, 2022

View reviewed changes

AdityaSripal approved these changes Jun 7, 2022

View reviewed changes

AdityaSripal reviewed Jun 7, 2022

View reviewed changes

damiannolan added 4 commits June 7, 2022 17:36

renaming RegisterDistributionAddress rpc to RegisterPayee

e4e7d01

renaming RegisterDistributionAddress to RegisterPayee

9f77b1d

updating godocs and field ordering

b53d8af

updating inline comment

3b2ccda

damiannolan changed the title ~~feat: adding RegisterDistributionAddress rpc~~ feat: adding RegisterPayee rpc to ics29 fee middleware Jun 7, 2022

AdityaSripal approved these changes Jun 8, 2022

View reviewed changes

Merge branch 'main' into damian/1477-ics29-register-distaddr

843cec9

damiannolan enabled auto-merge (squash) June 8, 2022 13:23

damiannolan merged commit 040f2ea into main Jun 8, 2022

damiannolan deleted the damian/1477-ics29-register-distaddr branch June 8, 2022 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adding RegisterPayee rpc to ics29 fee middleware #1491

feat: adding RegisterPayee rpc to ics29 fee middleware #1491

damiannolan commented Jun 6, 2022 •

edited

Loading

codecov-commenter commented Jun 6, 2022 •

edited

Loading

crodriguezvega left a comment

crodriguezvega Jun 6, 2022

damiannolan Jun 7, 2022

crodriguezvega Jun 6, 2022

damiannolan Jun 7, 2022

crodriguezvega Jun 6, 2022

damiannolan Jun 7, 2022

damiannolan Jun 7, 2022

AdityaSripal Jun 7, 2022

damiannolan Jun 7, 2022

AdityaSripal left a comment

AdityaSripal Jun 7, 2022

AdityaSripal Jun 7, 2022

AdityaSripal Jun 7, 2022

crodriguezvega Jun 7, 2022

damiannolan Jun 7, 2022

AdityaSripal left a comment

feat: adding RegisterPayee rpc to ics29 fee middleware #1491

feat: adding RegisterPayee rpc to ics29 fee middleware #1491

Conversation

damiannolan commented Jun 6, 2022 • edited Loading

Description

codecov-commenter commented Jun 6, 2022 • edited Loading

Codecov Report

crodriguezvega 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

AdityaSripal 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

AdityaSripal left a comment

Choose a reason for hiding this comment

damiannolan commented Jun 6, 2022 •

edited

Loading

codecov-commenter commented Jun 6, 2022 •

edited

Loading