Skip to content

Commit

Permalink
fix: address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
JimLarson committed Oct 3, 2021
1 parent 40c3d1d commit 8af3e15
Show file tree
Hide file tree
Showing 10 changed files with 45 additions and 57 deletions.
10 changes: 5 additions & 5 deletions golang/cosmos/x/lien/keeper/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@ func maxCoins(a, b sdk.Coins) sdk.Coins {
indexA, indexB := 0, 0
for indexA < len(a) && indexB < len(b) {
coinA, coinB := a[indexA], b[indexB]
switch strings.Compare(coinA.Denom, coinB.Denom) {
case -1: // A < B
switch cmp := strings.Compare(coinA.Denom, coinB.Denom); {
case cmp < 0: // A < B
max = append(max, coinA)
indexA++
case 0: // A == B
case cmp == 0: // A == B
maxCoin := coinA
if coinB.IsGTE(maxCoin) {
maxCoin = coinB
Expand All @@ -32,7 +32,7 @@ func maxCoins(a, b sdk.Coins) sdk.Coins {
}
indexA++
indexB++
case 1: // A > B
case cmp > 0: // A > B
max = append(max, coinB)
indexB++
}
Expand Down Expand Up @@ -61,7 +61,7 @@ type LienAccount struct {
var _ vestexported.VestingAccount = LienAccount{}

// LockedCoins implements the method from the VestingAccount interface.
// It takes the maximum of the coins locked for liens and the cons
// It takes the maximum of the coins locked for liens and the coins
// locked in the wrapped VestingAccount.
func (la LienAccount) LockedCoins(ctx sdk.Context) sdk.Coins {
wrappedLocked := la.VestingAccount.LockedCoins(ctx)
Expand Down
2 changes: 1 addition & 1 deletion golang/cosmos/x/lien/keeper/account_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ func TestWrap(t *testing.T) {
t.Fatalf("unwrapper did not produce a base account: %+v", unwrapped)
}
if baseAccount.AccountNumber != 17 {
t.Errorf("wong account number %d, want 17", baseAccount.AccountNumber)
t.Errorf("wrong account number %d, want 17", baseAccount.AccountNumber)
}
unwrap2 := wrapper.Unwrap(baseAccount)
_, ok = unwrap2.(*authtypes.BaseAccount)
Expand Down
12 changes: 2 additions & 10 deletions golang/cosmos/x/lien/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,6 @@ type Keeper interface {
GetAccountState(ctx sdk.Context, addr sdk.AccAddress) AccountState
}

// stakingToken is the single denomination used for staking.
// The lien API pays lip service to the idea that staking is done with
// arbitrary sdk.Coins, but the UnboindingDelegation contains only a bare
// sdk.Int, therefore the staking token must be a single implicit denom.
const stakingToken = "ubld"

// keeperImpl implements the lien keeper.
// The accountKeeper field must be the same one that the bankKeeper and
// stakingKeeper use.
Expand Down Expand Up @@ -143,17 +137,15 @@ func (lk keeperImpl) getBonded(ctx sdk.Context, addr sdk.AccAddress) sdk.Coins {
for _, d := range delegations {
validatorAddr, err := sdk.ValAddressFromBech32(d.ValidatorAddress)
if err != nil {
// XXX ???
panic(err)
}
validator, found := lk.stakingKeeper.GetValidator(ctx, validatorAddr)
if !found {
// XXX ???
panic("validator not found")
}
shares := d.GetShares()
tokens := validator.TokensFromShares(shares)
bonded = bonded.Add(sdk.NewCoin(stakingToken, tokens.RoundInt())) // XXX rounding?
bonded = bonded.Add(sdk.NewCoin(lk.stakingKeeper.BondDenom(ctx), tokens.RoundInt())) // XXX rounding?
}
return bonded
}
Expand All @@ -167,7 +159,7 @@ func (lk keeperImpl) getUnbonding(ctx sdk.Context, addr sdk.AccAddress) sdk.Coin
for _, e := range u.Entries {
amt = amt.Add(e.Balance)
}
unbonding = unbonding.Add(sdk.NewCoin(stakingToken, amt))
unbonding = unbonding.Add(sdk.NewCoin(lk.stakingKeeper.BondDenom(ctx), amt))
}
return unbonding
}
Expand Down
2 changes: 1 addition & 1 deletion golang/cosmos/x/lien/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ func TestAccountState(t *testing.T) {
state = keeper.GetAccountState(ctx, addr1)
wantState = AccountState{Liened: amt1}
if !state.IsEqual(wantState) {
t.Errorf("GetAccountState() of lein only got %v, want %v", state, wantState)
t.Errorf("GetAccountState() of lien only got %v, want %v", state, wantState)
}

// total and lien
Expand Down
47 changes: 21 additions & 26 deletions golang/cosmos/x/lien/lien.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,20 @@ type portHandler struct {

// portMessage is a struct that any lien bridge message can unmarshal into.
type portMessage struct {
Type string `json:"type"`
Address string `json:"address"`
Denom string `json:"denom"`
Amount string `json:"amount"`
Type string `json:"type"`
Address string `json:"address"`
Denom string `json:"denom"`
Amount sdk.Int `json:"amount"`
}

// msgAccountState marshals into the AccountState message for the lien bridge.
type msgAccountState struct {
CurrentTime string `json:"currentTime"`
Total string `json:"total"`
Bonded string `json:"bonded"`
Unbonding string `json:"unbonding"`
Locked string `json:"locked"`
Liened string `json:"liened"`
CurrentTime int64 `json:"currentTime"`
Total sdk.Int `json:"total"`
Bonded sdk.Int `json:"bonded"`
Unbonding sdk.Int `json:"unbonding"`
Locked sdk.Int `json:"locked"`
Liened sdk.Int `json:"liened"`
}

// NewPortHandler returns a port handler for the Keeper.
Expand Down Expand Up @@ -79,12 +79,12 @@ func (ch portHandler) handleGetAccountState(ctx sdk.Context, msg portMessage) (s
}
state := ch.keeper.GetAccountState(ctx, addr)
reply := msgAccountState{
CurrentTime: ctx.BlockTime().String(), // REVIEWER: what format?
Total: state.Total.AmountOf(denom).String(),
Bonded: state.Bonded.AmountOf(denom).String(),
Unbonding: state.Unbonding.AmountOf(denom).String(),
Locked: state.Locked.AmountOf(denom).String(),
Liened: state.Liened.AmountOf(denom).String(),
CurrentTime: ctx.BlockTime().Unix(),
Total: state.Total.AmountOf(denom),
Bonded: state.Bonded.AmountOf(denom),
Unbonding: state.Unbonding.AmountOf(denom),
Locked: state.Locked.AmountOf(denom),
Liened: state.Liened.AmountOf(denom),
}
bz, err := json.Marshal(&reply)
if err != nil {
Expand All @@ -104,19 +104,14 @@ func (ch portHandler) handleSetTotal(ctx sdk.Context, msg portMessage) (string,
if err = sdk.ValidateDenom(denom); err != nil {
return "", fmt.Errorf("invalid denom %s: %w", denom, err)
}
var amount sdk.Int
err = amount.UnmarshalJSON([]byte(msg.Amount))
if err != nil {
return "", fmt.Errorf("cannot decode lien amount %s: %w", msg.Amount, err)
}
lien := ch.keeper.GetLien(ctx, addr)
oldAmount := lien.GetCoins().AmountOf(denom)
if amount.Equal(oldAmount) {
if msg.Amount.Equal(oldAmount) {
// no-op, no need to do anything
return "true", nil
} else if amount.LT(oldAmount) {
} else if msg.Amount.LT(oldAmount) {
// always okay to reduce liened amount
diff := sdk.NewCoin(denom, oldAmount.Sub(amount))
diff := sdk.NewCoin(denom, oldAmount.Sub(msg.Amount))
lien.Coins = lien.GetCoins().Sub(sdk.NewCoins(diff))
ch.keeper.SetLien(ctx, addr, lien)
return "true", nil
Expand All @@ -129,10 +124,10 @@ func (ch portHandler) handleSetTotal(ctx sdk.Context, msg portMessage) (string,
locked := state.Locked.AmountOf(denom)
unbonded := total.Sub(bonded).Sub(unbonding)
unlocked := total.Sub(locked)
if amount.GT(unbonded) || amount.GT(unlocked) {
if msg.Amount.GT(unbonded) || msg.Amount.GT(unlocked) {
return "false", nil
}
diff := sdk.NewCoin(denom, amount.Sub(oldAmount))
diff := sdk.NewCoin(denom, msg.Amount.Sub(oldAmount))
lien.Coins = lien.GetCoins().Add(diff)
ch.keeper.SetLien(ctx, addr, lien)
return "true", nil
Expand Down
10 changes: 8 additions & 2 deletions golang/cosmos/x/lien/lien_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,15 @@ func TestGetAccountState(t *testing.T) {
if err != nil {
t.Fatalf("cannot unmarshal reply %s: %v", reply, err)
}
acctState.CurrentTime = "" // TODO: make predictable
acctState.CurrentTime = int64(123456789)
zero := sdk.NewInt(0)
want := msgAccountState{
CurrentTime: "", Total: "123", Bonded: "0", Unbonding: "0", Locked: "0", Liened: "0",
CurrentTime: acctState.CurrentTime,
Total: sdk.NewInt(123),
Bonded: zero,
Unbonding: zero,
Locked: zero,
Liened: zero,
}
if acctState != want {
t.Errorf("got account state %v, want %v", acctState, want)
Expand Down
6 changes: 3 additions & 3 deletions golang/cosmos/x/lien/spec/01_concepts.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ order: 1
## Notes on Terminology

* "Bonded", "delegated", and "staked" are all synonyms.
* "Unlocking" is a time-delayted process by which tokens become available for
* "Unlocking" is a time-delayed process by which tokens become available for
withdrawal. "Vesting" is a time-delayed process where tokens change
ownership, and is subject to "clawback" which stops the process. The
cosmos-sdk implements several kinds of "vesting account" which implement
Expand Down Expand Up @@ -73,7 +73,7 @@ we cannot force tokens to remain bonded, as they may be involuntarily
unbonded when a validator retires. The best we can do is that when
the higher layers wish to increase the total liened amount, the new
total must be less than the bonded tokens and less than the unlocked
tokens. Liens can be lifted at any time,
tokens. Liens can be lifted at any time.

The liened amount is *not* reduced by slashing, and if slashing reduces
the account balance below the liened amount, the account effectively has
Expand Down Expand Up @@ -113,7 +113,7 @@ which gives rise to the derived quantities:

## Implementing Lien Encumbrance

Unliened tokens may not be transferred out of an account. Rather than
Liened tokens may not be transferred out of an account. Rather than
modifying the `x/bank` module to implement a new type of containment,
we leverage the existing mechanism to prevent locked tokens from being
transferred.
Expand Down
6 changes: 3 additions & 3 deletions golang/cosmos/x/lien/types/accountkeeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ var DefaultAccountWrapper = AccountWrapper{
Unwrap: identWrap,
}

// WrappedAccountKeeper wraps an account AccountKeeper insert an outer wrapper
// to modify the behavior of stored accounts.
// WrappedAccountKeeper wraps an account AccountKeeper transform accounts
// when reading or writing accounts from/to storage.
// Applies the Wrap function when reading accounts from the store and applies
// the Unwrap function when writing them to the store.
//
Expand Down Expand Up @@ -72,7 +72,7 @@ func (wak *WrappedAccountKeeper) SetAccount(ctx sdk.Context, acc authtypes.Accou
wak.AccountKeeper.SetAccount(ctx, unwrappedAcc)
}

// SetWrapper updates the AcountWrapper.
// SetWrapper updates the AccountWrapper.
// We need to modify the wrapper after it's created for the planned use,
// since there is a circular dependency in the creation of the WrappedAccountKeeper
// and the lien.Keeper.
Expand Down
1 change: 1 addition & 0 deletions golang/cosmos/x/lien/types/expected_keepers.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ type BankKeeper interface {
}

type StakingKeeper interface {
BondDenom(ctx sdk.Context) string
GetDelegatorDelegations(ctx sdk.Context, delegator sdk.AccAddress, maxRetrieve uint16) []stakingTypes.Delegation
GetUnbondingDelegations(ctx sdk.Context, delegator sdk.AccAddress, maxRetrieve uint16) []stakingTypes.UnbondingDelegation
GetValidator(ctx sdk.Context, addr sdk.ValAddress) (stakingTypes.Validator, bool)
Expand Down
6 changes: 0 additions & 6 deletions packages/cosmic-swingset/src/block-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const DELIVER_INBOUND = 'DELIVER_INBOUND';
const END_BLOCK = 'END_BLOCK';
const COMMIT_BLOCK = 'COMMIT_BLOCK';
const IBC_EVENT = 'IBC_EVENT';
const LIEN_GET_LIENED_AMOUNT = 'LIEN_GET_LIENED_AMOUNT';
const PLEASE_PROVISION = 'PLEASE_PROVISION';
const VBANK_BALANCE_UPDATE = 'VBANK_BALANCE_UPDATE';

Expand Down Expand Up @@ -129,11 +128,6 @@ export default function makeBlockManager({
break;
}

case LIEN_GET_LIENED_AMOUNT: {
const lockedAmount = 0n;
return await Promise.resolve(JSON.stringify(String(lockedAmount)));
}

case COMMIT_BLOCK: {
verboseBlocks && console.info('block', action.blockHeight, 'commit');
if (action.blockHeight !== computedHeight) {
Expand Down

0 comments on commit 8af3e15

Please sign in to comment.