Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: #258 Register Counterparty Address #376

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions docs/ibc/proto-docs.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@
- [ibc/applications/fee/v1/tx.proto](#ibc/applications/fee/v1/tx.proto)
- [MsgEscrowPacketFee](#ibc.applications.fee.v1.MsgEscrowPacketFee)
- [MsgEscrowPacketFeeResponse](#ibc.applications.fee.v1.MsgEscrowPacketFeeResponse)
- [MsgRegisterCounterPartyAddressResponse](#ibc.applications.fee.v1.MsgRegisterCounterPartyAddressResponse)
- [MsgRegisterCounterpartyAddress](#ibc.applications.fee.v1.MsgRegisterCounterpartyAddress)
- [MsgRegisterCounterpartyAddressResponse](#ibc.applications.fee.v1.MsgRegisterCounterpartyAddressResponse)

- [Msg](#ibc.applications.fee.v1.Msg)

Expand Down Expand Up @@ -915,16 +915,6 @@ MsgEscrowPacketFeeResponse defines the response type for Msg/EscrowPacketFee



<a name="ibc.applications.fee.v1.MsgRegisterCounterPartyAddressResponse"></a>

### MsgRegisterCounterPartyAddressResponse
MsgRegisterCounterPartyAddressResponse defines the Msg/RegisterCounteryPartyAddress response type






<a name="ibc.applications.fee.v1.MsgRegisterCounterpartyAddress"></a>

### MsgRegisterCounterpartyAddress
Expand All @@ -940,6 +930,16 @@ MsgRegisterCounterpartyAddress is the request type for registering the counter p




<a name="ibc.applications.fee.v1.MsgRegisterCounterpartyAddressResponse"></a>

### MsgRegisterCounterpartyAddressResponse
MsgRegisterCounterpartyAddressResponse defines the Msg/RegisterCounterypartyAddress response type





<!-- end messages -->

<!-- end enums -->
Expand All @@ -954,7 +954,7 @@ Msg defines the ibc/fee Msg service.

| Method Name | Request Type | Response Type | Description | HTTP Verb | Endpoint |
| ----------- | ------------ | ------------- | ------------| ------- | -------- |
| `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, latest counterparty address 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, latest counterparty address is always used. | |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| `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, latest counterparty address 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 on the source chain 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 previous counterparty address will be overwritten by the new counterparty address. | |

for clarity?

| `EscrowPacketFee` | [MsgEscrowPacketFee](#ibc.applications.fee.v1.MsgEscrowPacketFee) | [MsgEscrowPacketFeeResponse](#ibc.applications.fee.v1.MsgEscrowPacketFeeResponse) | EscrowPacketFee defines a rpc handler method for MsgEscrowPacketFee EscrowPacketFee is an open callback that may be called by any module/user that wishes to escrow funds in order to incentivize the relaying of the given packet. | |

<!-- end services -->
Expand Down
31 changes: 21 additions & 10 deletions modules/apps/29-fee/keeper/keeper.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
package keeper

/*
import (
"github.com/tendermint/tendermint/libs/log"

"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
capabilitykeeper "github.com/cosmos/cosmos-sdk/x/capability/keeper"
capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types"
paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
"github.com/cosmos/ibc-go/modules/apps/transfer/types"
"github.com/cosmos/ibc-go/modules/apps/29-fee/types"
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
host "github.com/cosmos/ibc-go/modules/core/24-host"
)

Expand All @@ -25,7 +23,6 @@ type Keeper struct {
scopedKeeper capabilitykeeper.ScopedKeeper
}


// NewKeeper creates a new 29-fee Keeper instance
func NewKeeper(
cdc codec.BinaryCodec, key sdk.StoreKey, paramSpace paramtypes.Subspace,
Expand All @@ -49,6 +46,7 @@ func (k Keeper) Logger(ctx sdk.Context) log.Logger {
return ctx.Logger().With("module", "x/"+host.ModuleName+"-"+types.ModuleName)
}

/*
// IsBound checks if the transfer module is already bound to the desired port
func (k Keeper) IsBound(ctx sdk.Context, portID string) bool {
_, ok := k.scopedKeeper.GetCapability(ctx, host.PortPath(portID))
Expand All @@ -62,12 +60,6 @@ func (k Keeper) BindPort(ctx sdk.Context, portID string) error {
return k.ClaimCapability(ctx, cap, host.PortPath(portID))
}

// GetPort returns the portID for the transfer module. Used in ExportGenesis
func (k Keeper) GetPort(ctx sdk.Context) string {
store := ctx.KVStore(k.storeKey)
return string(store.Get(types.PortKey))
}

// SetPort sets the portID for the transfer module. Used in InitGenesis
func (k Keeper) SetPort(ctx sdk.Context, portID string) {
store := ctx.KVStore(k.storeKey)
Expand All @@ -85,3 +77,22 @@ func (k Keeper) ClaimCapability(ctx sdk.Context, cap *capabilitytypes.Capability
return k.scopedKeeper.ClaimCapability(ctx, cap, name)
}
*/

// SetCounterPartyAddress maps the counterparty relayer address to the source relayer address
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CounterParty -> Counterparty

func (k Keeper) SetCounterpartyAddress(ctx sdk.Context, SourceAddress, CounterpartyAddress string) {
seantking marked this conversation as resolved.
Show resolved Hide resolved
store := ctx.KVStore(k.storeKey)
store.Set(types.KeySourceAddress(SourceAddress), []byte(CounterpartyAddress))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mapping is incorrect. We are mapping the address on the destination chain -> address on the source chain.

It is the responsibility of the receiving chain to authenticate that the message was received from owner of address. The receiving chain must store the mapping from: address -> counterpartyAddress for the given channel. Then, onRecvPacket of the destination fee middleware can query for the counterparty address of the recvPacket message sender in order to get the source address of the forward relayer. This source address is what will get embedded in the acknowledgement.

I'd use spec terminology of address -> counterpartyAddress, but the godoc should specify this is the address on the source chain of the forward relayer and that this is used in processing a packet recv on the destination chain

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. wdyt?

}

// GetCounterPartyAddress gets the relayer counterparty address given a source address
func (k Keeper) GetCounterPartyAddress(ctx sdk.Context, sourceAddress sdk.AccAddress) (sdk.AccAddress, bool) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CounterParty -> Counterparty

store := ctx.KVStore(k.storeKey)
key := types.KeySourceAddress(sourceAddress.String())

if !store.Has(key) {
// TODO: add logging here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is logging useful here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I was thinking of a DEBUG log for the rare case that you're trying to figure out why something isn't working.

Hadn't put much thought into it though tbh. Patterns for logging are always a funny topic of conversation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking there might be times when the key not existing makes sense. Might make more sense to log where this function is called from (if we are certain the key should exist)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to leave it out for now. 👍

return []byte{}, false
}

return store.Get(key), true
}
14 changes: 0 additions & 14 deletions modules/apps/29-fee/keeper/keeper_test.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,10 @@
package keeper_test

/*
import (
"testing"

"github.com/stretchr/testify/suite"
"github.com/tendermint/tendermint/crypto"

"github.com/cosmos/cosmos-sdk/baseapp"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/ibc-go/modules/apps/transfer/types"
ibctesting "github.com/cosmos/ibc-go/testing"
)

Expand All @@ -31,15 +26,6 @@ func (suite *KeeperTestSuite) SetupTest() {
suite.chainC = suite.coordinator.GetChain(ibctesting.GetChainID(2))
}

func NewFeePath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path {
path := ibctesting.NewPath(chainA, chainB)
path.EndpointA.ChannelConfig.PortID = ibctesting.FeePort
path.EndpointB.ChannelConfig.PortID = ibctesting.FeePort

return path
}

func TestKeeperTestSuite(t *testing.T) {
suite.Run(t, new(KeeperTestSuite))
}
*/
33 changes: 31 additions & 2 deletions modules/apps/29-fee/keeper/msg_server.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,33 @@
package keeper

// TODO
//var _ types.MsgServer = Keeper{}
import (
"context"

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/modules/apps/29-fee/types"
)

var _ types.MsgServer = Keeper{}

// 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 relayer, in which case, latest counterparty address is always used.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AdityaSripal I took this to mean that the address can be overwritten by calling RegisterCounterPartyAddress more than once. Is that correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct

func (k Keeper) RegisterCounterpartyAddress(goCtx context.Context, msg *types.MsgRegisterCounterpartyAddress) (*types.MsgRegisterCounterpartyAddressResponse, error) {
ctx := sdk.UnwrapSDKContext(goCtx)

k.SetCounterpartyAddress(
ctx, msg.Address, msg.CounterpartyAddress,
)

k.Logger(ctx).Info("Registering counterparty address for relayer with:", "Source address:", msg.Address, "With counterparty address:", msg.CounterpartyAddress)
seantking marked this conversation as resolved.
Show resolved Hide resolved

return &types.MsgRegisterCounterpartyAddressResponse{}, nil
}

// EscrowPacketFee defines a rpc handler method for MsgEscrowPacketFee
// EscrowPacketFee is an open callback that may be called by any module/user that wishes to escrow funds in order to
// incentivize the relaying of the given packet.
func (k Keeper) EscrowPacketFee(goCtx context.Context, msg *types.MsgEscrowPacketFee) (*types.MsgEscrowPacketFeeResponse, error) {
return &types.MsgEscrowPacketFeeResponse{}, nil
}
2 changes: 0 additions & 2 deletions modules/apps/29-fee/types/expected_keepers.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package types

/*
import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/auth/types"
Expand Down Expand Up @@ -47,4 +46,3 @@ type ConnectionKeeper interface {
type PortKeeper interface {
BindPort(ctx sdk.Context, portID string) *capabilitytypes.Capability
}
*/
10 changes: 10 additions & 0 deletions modules/apps/29-fee/types/keys.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,25 @@
package types

import fmt "fmt"
seantking marked this conversation as resolved.
Show resolved Hide resolved

const (
// ModuleName defines the 29-fee name
ModuleName = "ibcfee"

// StoreKey is the store key string for IBC transfer
StoreKey = ModuleName

// PortKey is the port id that is wrapped by fee middleware
PortKey = "feetransfer"

// RouterKey is the message route for IBC transfer
RouterKey = ModuleName

// QuerierRoute is the querier route for IBC transfer
QuerierRoute = ModuleName
)

// Key for relayer source address -> counteryparty address mapping
func KeySourceAddress(sourceAddress string) []byte {
return []byte(fmt.Sprintf("relayerSourceAddress/%s", sourceAddress))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be useful to make relayerSourceAddress/ a variable. We might need to do range queries in which case we would need this value

}
36 changes: 22 additions & 14 deletions modules/apps/29-fee/types/msgs.go
Original file line number Diff line number Diff line change
@@ -1,32 +1,40 @@
package types

/*
import (
"strings"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
host "github.com/cosmos/ibc-go/modules/core/24-host"
)

// NewMsg
func NewMsg() *Msg {
return &Msg{
}
// msg types
const (
TypeMsgRegisterCounterPartyAddress = "registerCounterPartyAddress"
)

// NewMsgRegisterCounterPartyAddress
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

godoc

func NewMsgRegisterCounterpartyAddress(sourceAddress, counterpartyAddress string) *MsgRegisterCounterpartyAddress {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
return &MsgRegisterCounterpartyAddress{Address: sourceAddress, CounterpartyAddress: counterpartyAddress}
seantking marked this conversation as resolved.
Show resolved Hide resolved
}

// ValidateBasic performs a basic check of the Msg fields.
func (msg Msg) ValidateBasic() error {
// ValidateBasic performs a basic check of the Msg fields
func (msg MsgRegisterCounterpartyAddress) ValidateBasic() error {
_, err := sdk.AccAddressFromBech32(msg.Address)
if err != nil {
return sdkerrors.Wrap(err, "Incorrect source relayer address")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: golang error string best practice

Error strings should not start with a capital letter because they'll often be prefixed before printing:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the error message should indicate it failed to convert to SDK bech32 addr?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated. wdyt?

}

_, err = sdk.AccAddressFromBech32(msg.CounterpartyAddress)
if err != nil {
return sdkerrors.Wrap(err, "Incorrect counterparty relayer address")
}

return nil
}

// GetSigners implements sdk.Msg
func (msg MsgTransfer) GetSigners() []sdk.AccAddress {
signer, err := sdk.AccAddressFromBech32(msg.Sender)
func (msg MsgRegisterCounterpartyAddress) GetSigners() []sdk.AccAddress {
signer, err := sdk.AccAddressFromBech32(msg.Address)
if err != nil {
panic(err)
}
return []sdk.AccAddress{signer}
}
*/
40 changes: 21 additions & 19 deletions modules/apps/29-fee/types/msgs_test.go
Original file line number Diff line number Diff line change
@@ -1,36 +1,38 @@
package types

/*
import (
"fmt"
"testing"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/stretchr/testify/require"
"github.com/tendermint/tendermint/crypto/secp256k1"
)

"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
sdk "github.com/cosmos/cosmos-sdk/types"
var (
validAadr1 = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validAadr1 -> validAddr1?

I'm also fine with just a validAddr and invalidAddr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

validAadr2 = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address()).String()
invalidAadr1 = "invalid_address"
invalidAadr2 = "invalid_address"
)

func (suite *TypesTestSuite) TestMsgValidateBasic() {
// TestMsgTransferValidation tests ValidateBasic for MsgTransfer
func TestMsgRegisterCountepartyAddressValidation(t *testing.T) {
testCases := []struct {
name string
msg *types.Msg
msg *MsgRegisterCounterpartyAddress
expPass bool
}{
{"", types.NewMsg(), true},
{"validate with correct sdk.AccAddress", NewMsgRegisterCounterpartyAddress(validAadr1, validAadr2), true},
{"validate with incorrect source relayer address", NewMsgRegisterCounterpartyAddress(invalidAadr1, validAadr2), false},
{"validate with incorrect counterparty source relayer address", NewMsgRegisterCounterpartyAddress(validAadr1, invalidAadr2), false},
}

for _, tc := range testCases {
tc := tc

suite.Run(tc.name, func() {
err := tc.msg.ValidateBasic()
if tc.expPass {
suite.Require().NoError(err)
} else {
suite.Require().Error(err)
}
})
for i, tc := range testCases {
err := tc.msg.ValidateBasic()
if tc.expPass {
require.NoError(t, err, "valid test case %d failed: %s", i, tc.name)
} else {
require.Error(t, err, "invalid test case %d passed: %s", i, tc.name)
}
}
}
*/
Loading