Skip to content

Commit 3eb46a2

Browse files
Vvaradinovmergify[bot]
authored andcommitted
imp: represent unlimited approvals with MaxUint256 value (#3454)
* imp: represent unlimited approvals with a nil value * CHANGELOG * Update CHANGELOG.md * fix: updated unlimited spending limit to be represented with the MaxInt64 * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com> * Update CHANGELOG.md Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com> * fix: lint * Update modules/apps/transfer/types/transfer_authorization.go * fix: update failing test and add test case suggested in review * fix: moved isAllowedAddress check before coin loop * fix: use maxUint256 case so it aligns with what's being passed from the EVM extension * refactor transfer authz to remove coins iteration in favour of explicit amount checking * make format * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Damian Nolan <damiannolan@gmail.com> * fix: add the Authorization to Updated. * moving allowlist check to before spend limit logic * Apply suggestions from code review * fix: add comment to spend limit check * review feedback * Update modules/apps/transfer/types/transfer_authorization.go Co-authored-by: Damian Nolan <damiannolan@gmail.com> * Update docs/apps/transfer/authorizations.md * fix: changed to new sentinel value name --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com> Co-authored-by: Damian Nolan <damiannolan@gmail.com> (cherry picked from commit 7e6eb4c) # Conflicts: # CHANGELOG.md # e2e/tests/core/client_test.go # e2e/testsuite/grpc_query.go # modules/apps/transfer/types/transfer_authorization.go
1 parent eec18b7 commit 3eb46a2

File tree

6 files changed

+787
-3
lines changed

6 files changed

+787
-3
lines changed

CHANGELOG.md

+6
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,12 @@ Ref: https://keepachangelog.com/en/1.0.0/
4646

4747
### Improvements
4848

49+
<<<<<<< HEAD
50+
=======
51+
* (tests) [\#3138](https://github.com/cosmos/ibc-go/pull/3138) Support benchmarks and fuzz tests through `testing.TB`.
52+
* (apps/transfer) [\#3454](https://github.com/cosmos/ibc-go/pull/3454) Support transfer authorization unlimited spending when the max `uint256` value is provided as limit.
53+
54+
>>>>>>> 7e6eb4c6 (imp: represent unlimited approvals with MaxUint256 value (#3454))
4955
### Features
5056

5157
### Bug Fixes

docs/apps/transfer/authorizations.md

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# `TransferAuthorization`
22

3-
`TransferAuthorization` implements the `Authorization` interface for `ibc.applications.transfer.v1.MsgTransfer`. It allows a granter to grant a grantee the privilege to submit MsgTransfer on its behalf. Please see the [Cosmos SDK docs](https://docs.cosmos.network/v0.47/modules/authz) for more details on granting privileges via the `x/authz` module.
3+
`TransferAuthorization` implements the `Authorization` interface for `ibc.applications.transfer.v1.MsgTransfer`. It allows a granter to grant a grantee the privilege to submit `MsgTransfer` on its behalf. Please see the [Cosmos SDK docs](https://docs.cosmos.network/v0.47/modules/authz) for more details on granting privileges via the `x/authz` module.
44

55
More specifically, the granter allows the grantee to transfer funds that belong to the granter over a specified channel.
66

@@ -13,7 +13,7 @@ It takes:
1313

1414
- a `SourcePort` and a `SourceChannel` which together comprise the unique transfer channel identifier over which authorized funds can be transferred.
1515

16-
- a `SpendLimit` that specifies the maximum amount of tokens the grantee can spend. The `SpendLimit` is updated as the tokens are spent. This `SpendLimit` may also be updated to increase or decrease the limit as the granter wishes.
16+
- a `SpendLimit` that specifies the maximum amount of tokens the grantee can transfer. The `SpendLimit` is updated as the tokens are transfered, unless the sentinel value of the maximum value for a 256-bit unsigned integer (i.e. 2^256 - 1) is used for the amount, in which case the `SpendLimit` will not be updated (please be aware that using this sentinel value will grant the grantee the privilege to transfer **all** the tokens of a given denomination available at the granter's account). The helper function `UnboundedSpendLimit` in the `types` package of the `transfer` module provides the sentinel value that can be used. This `SpendLimit` may also be updated to increase or decrease the limit as the granter wishes.
1717

1818
- an `AllowList` list that specifies the list of addresses that are allowed to receive funds. If this list is empty, then all addresses are allowed to receive funds from the `TransferAuthorization`.
1919

e2e/tests/core/client_test.go

+335
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,335 @@
1+
package e2e
2+
3+
import (
4+
"context"
5+
"fmt"
6+
"sort"
7+
"strings"
8+
"testing"
9+
"time"
10+
11+
"github.com/cometbft/cometbft/crypto/tmhash"
12+
tmjson "github.com/cometbft/cometbft/libs/json"
13+
"github.com/cometbft/cometbft/privval"
14+
tmproto "github.com/cometbft/cometbft/proto/tendermint/types"
15+
tmprotoversion "github.com/cometbft/cometbft/proto/tendermint/version"
16+
tmtypes "github.com/cometbft/cometbft/types"
17+
tmversion "github.com/cometbft/cometbft/version"
18+
"github.com/cosmos/cosmos-sdk/client/grpc/tmservice"
19+
"github.com/strangelove-ventures/interchaintest/v7/chain/cosmos"
20+
"github.com/strangelove-ventures/interchaintest/v7/ibc"
21+
test "github.com/strangelove-ventures/interchaintest/v7/testutil"
22+
"github.com/stretchr/testify/suite"
23+
24+
"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
25+
26+
"github.com/cosmos/ibc-go/e2e/dockerutil"
27+
"github.com/cosmos/ibc-go/e2e/testsuite"
28+
"github.com/cosmos/ibc-go/e2e/testvalues"
29+
clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
30+
ibcexported "github.com/cosmos/ibc-go/v7/modules/core/exported"
31+
ibctm "github.com/cosmos/ibc-go/v7/modules/light-clients/07-tendermint"
32+
ibctesting "github.com/cosmos/ibc-go/v7/testing"
33+
ibcmock "github.com/cosmos/ibc-go/v7/testing/mock"
34+
)
35+
36+
const (
37+
invalidHashValue = "invalid_hash"
38+
)
39+
40+
func TestClientTestSuite(t *testing.T) {
41+
suite.Run(t, new(ClientTestSuite))
42+
}
43+
44+
type ClientTestSuite struct {
45+
testsuite.E2ETestSuite
46+
}
47+
48+
// Status queries the current status of the client
49+
func (s *ClientTestSuite) Status(ctx context.Context, chain ibc.Chain, clientID string) (string, error) {
50+
queryClient := s.GetChainGRCPClients(chain).ClientQueryClient
51+
res, err := queryClient.ClientStatus(ctx, &clienttypes.QueryClientStatusRequest{
52+
ClientId: clientID,
53+
})
54+
if err != nil {
55+
return "", err
56+
}
57+
58+
return res.Status, nil
59+
}
60+
61+
func (s *ClientTestSuite) TestClientUpdateProposal_Succeeds() {
62+
t := s.T()
63+
ctx := context.TODO()
64+
65+
var (
66+
pathName string
67+
relayer ibc.Relayer
68+
subjectClientID string
69+
substituteClientID string
70+
// set the trusting period to a value which will still be valid upon client creation, but invalid before the first update
71+
badTrustingPeriod = time.Duration(time.Second * 10)
72+
)
73+
74+
t.Run("create substitute client with correct trusting period", func(t *testing.T) {
75+
relayer, _ = s.SetupChainsRelayerAndChannel(ctx)
76+
77+
// TODO: update when client identifier created is accessible
78+
// currently assumes first client is 07-tendermint-0
79+
substituteClientID = clienttypes.FormatClientIdentifier(ibcexported.Tendermint, 0)
80+
81+
// TODO: replace with better handling of path names
82+
pathName = fmt.Sprintf("%s-path-%d", s.T().Name(), 0)
83+
pathName = strings.ReplaceAll(pathName, "/", "-")
84+
})
85+
86+
chainA, chainB := s.GetChains()
87+
chainAWallet := s.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount)
88+
89+
t.Run("create subject client with bad trusting period", func(t *testing.T) {
90+
createClientOptions := ibc.CreateClientOptions{
91+
TrustingPeriod: badTrustingPeriod.String(),
92+
}
93+
94+
s.SetupClients(ctx, relayer, createClientOptions)
95+
96+
// TODO: update when client identifier created is accessible
97+
// currently assumes second client is 07-tendermint-1
98+
subjectClientID = clienttypes.FormatClientIdentifier(ibcexported.Tendermint, 1)
99+
})
100+
101+
time.Sleep(badTrustingPeriod)
102+
103+
t.Run("update substitute client", func(t *testing.T) {
104+
s.UpdateClients(ctx, relayer, pathName)
105+
})
106+
107+
s.Require().NoError(test.WaitForBlocks(ctx, 1, chainA, chainB), "failed to wait for blocks")
108+
109+
t.Run("check status of each client", func(t *testing.T) {
110+
t.Run("substitute should be active", func(t *testing.T) {
111+
status, err := s.Status(ctx, chainA, substituteClientID)
112+
s.Require().NoError(err)
113+
s.Require().Equal(ibcexported.Active.String(), status)
114+
})
115+
116+
t.Run("subject should be expired", func(t *testing.T) {
117+
status, err := s.Status(ctx, chainA, subjectClientID)
118+
s.Require().NoError(err)
119+
s.Require().Equal(ibcexported.Expired.String(), status)
120+
})
121+
})
122+
123+
t.Run("pass client update proposal", func(t *testing.T) {
124+
proposal := clienttypes.NewClientUpdateProposal(ibctesting.Title, ibctesting.Description, subjectClientID, substituteClientID)
125+
s.ExecuteGovProposal(ctx, chainA, chainAWallet, proposal)
126+
})
127+
128+
t.Run("check status of each client", func(t *testing.T) {
129+
t.Run("substitute should be active", func(t *testing.T) {
130+
status, err := s.Status(ctx, chainA, substituteClientID)
131+
s.Require().NoError(err)
132+
s.Require().Equal(ibcexported.Active.String(), status)
133+
})
134+
135+
t.Run("subject should be active", func(t *testing.T) {
136+
status, err := s.Status(ctx, chainA, subjectClientID)
137+
s.Require().NoError(err)
138+
s.Require().Equal(ibcexported.Active.String(), status)
139+
})
140+
})
141+
}
142+
143+
func (s *ClientTestSuite) TestClient_Update_Misbehaviour() {
144+
t := s.T()
145+
ctx := context.TODO()
146+
147+
var (
148+
trustedHeight clienttypes.Height
149+
latestHeight clienttypes.Height
150+
clientState ibcexported.ClientState
151+
header testsuite.Header
152+
signers []tmtypes.PrivValidator
153+
validatorSet []*tmtypes.Validator
154+
maliciousHeader *ibctm.Header
155+
err error
156+
)
157+
158+
relayer, _ := s.SetupChainsRelayerAndChannel(ctx)
159+
chainA, chainB := s.GetChains()
160+
161+
s.Require().NoError(test.WaitForBlocks(ctx, 10, chainA, chainB))
162+
163+
t.Run("update clients", func(t *testing.T) {
164+
err := relayer.UpdateClients(ctx, s.GetRelayerExecReporter(), s.GetPathName(0))
165+
s.Require().NoError(err)
166+
167+
clientState, err = s.QueryClientState(ctx, chainA, ibctesting.FirstClientID)
168+
s.Require().NoError(err)
169+
})
170+
171+
t.Run("fetch trusted height", func(t *testing.T) {
172+
tmClientState, ok := clientState.(*ibctm.ClientState)
173+
s.Require().True(ok)
174+
175+
trustedHeight, ok = tmClientState.GetLatestHeight().(clienttypes.Height)
176+
s.Require().True(ok)
177+
})
178+
179+
t.Run("update clients", func(t *testing.T) {
180+
err := relayer.UpdateClients(ctx, s.GetRelayerExecReporter(), s.GetPathName(0))
181+
s.Require().NoError(err)
182+
183+
clientState, err = s.QueryClientState(ctx, chainA, ibctesting.FirstClientID)
184+
s.Require().NoError(err)
185+
})
186+
187+
t.Run("fetch client state latest height", func(t *testing.T) {
188+
tmClientState, ok := clientState.(*ibctm.ClientState)
189+
s.Require().True(ok)
190+
191+
latestHeight, ok = tmClientState.GetLatestHeight().(clienttypes.Height)
192+
s.Require().True(ok)
193+
})
194+
195+
t.Run("create validator set", func(t *testing.T) {
196+
var validators []*tmservice.Validator
197+
198+
t.Run("fetch block header at latest client state height", func(t *testing.T) {
199+
header, err = s.GetBlockHeaderByHeight(ctx, chainB, latestHeight.GetRevisionHeight())
200+
s.Require().NoError(err)
201+
})
202+
203+
t.Run("get validators at latest height", func(t *testing.T) {
204+
validators, err = s.GetValidatorSetByHeight(ctx, chainB, latestHeight.GetRevisionHeight())
205+
s.Require().NoError(err)
206+
})
207+
208+
t.Run("extract validator private keys", func(t *testing.T) {
209+
privateKeys := s.extractChainPrivateKeys(ctx, chainB)
210+
for i, pv := range privateKeys {
211+
pubKey, err := pv.GetPubKey()
212+
s.Require().NoError(err)
213+
214+
validator := tmtypes.NewValidator(pubKey, validators[i].VotingPower)
215+
216+
validatorSet = append(validatorSet, validator)
217+
signers = append(signers, pv)
218+
}
219+
})
220+
})
221+
222+
t.Run("create malicious header", func(t *testing.T) {
223+
valSet := tmtypes.NewValidatorSet(validatorSet)
224+
maliciousHeader, err = createMaliciousTMHeader(chainB.Config().ChainID, int64(latestHeight.GetRevisionHeight()), trustedHeight,
225+
header.GetTime(), valSet, valSet, signers, header)
226+
s.Require().NoError(err)
227+
})
228+
229+
t.Run("update client with duplicate misbehaviour header", func(t *testing.T) {
230+
rlyWallet := s.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount)
231+
msgUpdateClient, err := clienttypes.NewMsgUpdateClient(ibctesting.FirstClientID, maliciousHeader, rlyWallet.FormattedAddress())
232+
s.Require().NoError(err)
233+
234+
txResp, err := s.BroadcastMessages(ctx, chainA, rlyWallet, msgUpdateClient)
235+
s.Require().NoError(err)
236+
s.AssertValidTxResponse(txResp)
237+
})
238+
239+
t.Run("ensure client status is frozen", func(t *testing.T) {
240+
status, err := s.QueryClientStatus(ctx, chainA, ibctesting.FirstClientID)
241+
s.Require().NoError(err)
242+
s.Require().Equal(ibcexported.Frozen.String(), status)
243+
})
244+
}
245+
246+
// extractChainPrivateKeys returns a slice of tmtypes.PrivValidator which hold the private keys for all validator
247+
// nodes for a given chain.
248+
func (s *ClientTestSuite) extractChainPrivateKeys(ctx context.Context, chain *cosmos.CosmosChain) []tmtypes.PrivValidator {
249+
testContainers, err := dockerutil.GetTestContainers(s.T(), ctx, s.DockerClient)
250+
s.Require().NoError(err)
251+
252+
var filePvs []privval.FilePVKey
253+
var pvs []tmtypes.PrivValidator
254+
for _, container := range testContainers {
255+
isNodeForDifferentChain := !strings.Contains(container.Names[0], chain.Config().ChainID)
256+
isFullNode := strings.Contains(container.Names[0], fmt.Sprintf("%s-fn", chain.Config().ChainID))
257+
if isNodeForDifferentChain || isFullNode {
258+
continue
259+
}
260+
261+
validatorPrivKey := fmt.Sprintf("/var/cosmos-chain/%s/config/priv_validator_key.json", chain.Config().Name)
262+
privKeyFileContents, err := dockerutil.GetFileContentsFromContainer(ctx, s.DockerClient, container.ID, validatorPrivKey)
263+
s.Require().NoError(err)
264+
265+
var filePV privval.FilePVKey
266+
err = tmjson.Unmarshal(privKeyFileContents, &filePV)
267+
s.Require().NoError(err)
268+
filePvs = append(filePvs, filePV)
269+
}
270+
271+
// We sort by address as GetValidatorSetByHeight also sorts by address. When iterating over them, the index
272+
// will correspond to the correct ibcmock.PV.
273+
sort.SliceStable(filePvs, func(i, j int) bool {
274+
return filePvs[i].Address.String() < filePvs[j].Address.String()
275+
})
276+
277+
for _, filePV := range filePvs {
278+
pvs = append(pvs, &ibcmock.PV{
279+
PrivKey: &ed25519.PrivKey{Key: filePV.PrivKey.Bytes()},
280+
})
281+
}
282+
283+
return pvs
284+
}
285+
286+
// createMaliciousTMHeader creates a header with the provided trusted height with an invalid app hash.
287+
func createMaliciousTMHeader(chainID string, blockHeight int64, trustedHeight clienttypes.Height, timestamp time.Time, tmValSet, tmTrustedVals *tmtypes.ValidatorSet, signers []tmtypes.PrivValidator, oldHeader testsuite.Header) (*ibctm.Header, error) {
288+
tmHeader := tmtypes.Header{
289+
Version: tmprotoversion.Consensus{Block: tmversion.BlockProtocol, App: 2},
290+
ChainID: chainID,
291+
Height: blockHeight,
292+
Time: timestamp,
293+
LastBlockID: ibctesting.MakeBlockID(make([]byte, tmhash.Size), 10_000, make([]byte, tmhash.Size)),
294+
LastCommitHash: oldHeader.GetLastCommitHash(),
295+
ValidatorsHash: tmValSet.Hash(),
296+
NextValidatorsHash: tmValSet.Hash(),
297+
DataHash: tmhash.Sum([]byte(invalidHashValue)),
298+
ConsensusHash: tmhash.Sum([]byte(invalidHashValue)),
299+
AppHash: tmhash.Sum([]byte(invalidHashValue)),
300+
LastResultsHash: tmhash.Sum([]byte(invalidHashValue)),
301+
EvidenceHash: tmhash.Sum([]byte(invalidHashValue)),
302+
ProposerAddress: tmValSet.Proposer.Address, //nolint:staticcheck
303+
}
304+
305+
hhash := tmHeader.Hash()
306+
blockID := ibctesting.MakeBlockID(hhash, 3, tmhash.Sum([]byte(invalidHashValue)))
307+
voteSet := tmtypes.NewVoteSet(chainID, blockHeight, 1, tmproto.PrecommitType, tmValSet)
308+
309+
commit, err := tmtypes.MakeCommit(blockID, blockHeight, 1, voteSet, signers, timestamp)
310+
if err != nil {
311+
return nil, err
312+
}
313+
314+
signedHeader := &tmproto.SignedHeader{
315+
Header: tmHeader.ToProto(),
316+
Commit: commit.ToProto(),
317+
}
318+
319+
valSet, err := tmValSet.ToProto()
320+
if err != nil {
321+
return nil, err
322+
}
323+
324+
trustedVals, err := tmTrustedVals.ToProto()
325+
if err != nil {
326+
return nil, err
327+
}
328+
329+
return &ibctm.Header{
330+
SignedHeader: signedHeader,
331+
ValidatorSet: valSet,
332+
TrustedHeight: trustedHeight,
333+
TrustedValidators: trustedVals,
334+
}, nil
335+
}

0 commit comments

Comments
 (0)