Fix claim bug

[riley-stride]

Summary

1. Add input validation to redeem-stake
2. Rename whitelist => admin and make include both local-dev and testnet admin
3. Fix claim bug
   i. Remove claimableRecordIds from HostZone
   ii. Remove records in ack 6d5da1
   iii. Add PendingClaims data structure to enable ICA callbacks

Note:

• moving the admin addresses to params may not be a good solution because it would not allow us to check certain functions whose msg_servers are defined outside our custom modules (e.g. registerICA's msg server is defined in the ICAcontrollerkeeper). We can't access stakeibc params inside stakeibc/types, which is where ValidateBasic is defined, so we can't check sender addresses against admin param there. We'd need to do so in the message server. This works for the functions we defined, but not for those defined in other modules e.g. registerICA.

Changes (part 1 and 2)

Input validation:

[note: would ideally move all validation logic to ValidateBasic(), but we can't access the keeper from there, and we need the keeper to validate hostZone]

• Add a bech32prefix field to the hostZone proto (necessary for validating foreign zone addresses from Stride) and add bech32prefix args to all registerHostZone messages, types and invocations
• Copy zone address verification code from cosmos-sdk into utils.go and parametrize it by hostZone so we can invoke it for any hostZone when we'd like to check the validity of an arbitrary address (note this assumes len 20 addresses, so will fail with ICA accounts; should be OK, we don't need to redeem to those accounts but maybe worth revisiting)
• In msg_server_redeem_stake.go, check the receiver address is a valid address on the specified host zone, after checking the host zone exists.
• In message_redeem_stake.go, check amount is > 0

Test plan

To test input validation, attempt to submit an incorrectly formatted redeem-stake message!
The following redeem-stake command should succeed (be sure to run 1.sh and 2.sh beforehand as setup.

$STRIDE_CMD tx stakeibc redeem-stake 89 GAIA cosmos1g6qdx6kdhpf000afvvpte7hp0vnpzapuyxp8uf --from $STRIDE_VAL_ACCT --keyring-backend test --chain-id $STRIDE_CHAIN -y

The following should fail:

# stride prefix, nonexistent OK
$STRIDE_CMD tx stakeibc redeem-stake 89 GAIA stride1g6qdx6kdhpf000afvvpte7hp0vnpzapuyxp8uf --from $STRIDE_VAL_ACCT --keyring-backend test --chain-id $STRIDE_CHAIN -y
# stride prefix, exists on stride
$STRIDE_CMD tx stakeibc redeem-stake 89 GAIA stride159atdlc3ksl50g0659w5tq42wwer334ajl7xnq --from $STRIDE_VAL_ACCT --keyring-backend test --chain-id $STRIDE_CHAIN -y
# cosmos address with bad hash at end
$STRIDE_CMD tx stakeibc redeem-stake 89 GAIA cosmos1g6qdx6kdhpf000afvvpte7hp0vnpzapuyxp8ug --from $STRIDE_VAL_ACCT --keyring-backend test --chain-id $STRIDE_CHAIN -y
# cosmos addr too long
$STRIDE_CMD tx stakeibc redeem-stake 89 GAIA cosmos1g6qdx6kdhpf000afvvpte7hp0vnpzapuyxp8uff --from $STRIDE_VAL_ACCT --keyring-backend test --chain-id $STRIDE_CHAIN -y
# empty address 
$STRIDE_CMD tx stakeibc redeem-stake 89 GAIA " " --from $STRIDE_VAL_ACCT --keyring-backend test --chain-id $STRIDE_CHAIN -y
# too short
$STRIDE_CMD tx stakeibc redeem-stake 89 GAIA cosmos1g6qdx6kdhpf000afvvpte7hp0vnpzapuyxp8u --from $STRIDE_VAL_ACCT --keyring-backend test --chain-id $STRIDE_CHAIN -y
# bad host zone
$STRIDE_CMD tx stakeibc redeem-stake 89 GAIAA cosmos1g6qdx6kdhpf000afvvpte7hp0vnpzapuyxp8u --from $STRIDE_VAL_ACCT --keyring-backend test --chain-id $STRIDE_CHAIN -y

Changes (claim bug fix)

• Scaffold PendingClaims to enable ICA callbacks (ca71746). This maps IBC packet sequence number to a claimable record id (note: for now, this maps to a single record, but this could be extended to process multiple claims in a single transaction)
• Updates logic in ClaimUndelegatedTokens (x/stakeibc/keeper/msg_server_claim_undelegated_tokens.go)
  • claim records one-by-one
  • store a mapping from IBC packet sequence number to record id (enabling callback logic in the ack)
  • add checks (record exists, is claimable, host zone exists)
• add logic to HandleSend in ack to delete PendingClaims on failure, and to delete both PendingClaims and UserRedemptionRecords on success

Test plan (claim bug fix)

Run 1.sh, 2.sh, 3.sh then claim the first record with 4.sh
Run 3.sh twice more to create records two and three, claim record three with 4.sh
Run 4.sh one last time to claim record two

[scalalang2]

what is whitelist address ?
If this is what it need in advance. it would be better on the params.

[scalalang2]

I think, at least, this address should exist as a parameter in the module.
so that it must be changed only by governance proposal.

[riley-stride]

Great question! This is an admin account that's here for testnet purposes, good point about parameters. We appreciate the reviews, please keep them coming! By the way, if you want to get in touch please DM us on Discord, we'd love to chat more 👍

[riley-stride]

Should we move this check to the top?

[asalzmann]

Can you elaborate on this?

[riley-stride]

part of my PR that does validation on inputs was to check the validity of hostzones in each msg_server that has a hostZone in its msg (like this msg_server). I always do that at the top of the msg_server. Mostly for consistency

[asalzmann]

got it - I think this should still fail if the host zone doesn't exist! there isn't any substantive logic above it, just checks on UserRedemptionRecords. slight preference to keep this here so the sections of the function are

1. Checks on UserRedemptionRecord
2. Checks on all fields required for the ICA
3. Constructing and sending the transaction, updating state

[asalzmann]

lgtm overall! few minor questions