Remove Shares Concept from Unbond/Redelegate UX

[alexanderbez]

• Remove redundant msg* types from the LCD tests (they're already defined in staking)
• Provide better names to staking REST types.
• Update unbonding and redelegation messages/requests to accept an sdk.Coin amount instead of shares.
  • Update respective handlers to convert amount to shares.

I also have updates to update the queriers to return amounts instead of shares but I think that should be a separate PR.

closes: #3516

---

• 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.

• Wrote tests

• Updated relevant documentation (docs/)

• Added entries in PENDING.md with issue #

• rereviewed Files changed in the github PR explorer

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

Merging #3857 into develop will decrease coverage by 0.04%.
The diff coverage is 64.58%.

@@             Coverage Diff             @@
##           develop    #3857      +/-   ##
===========================================
- Coverage    60.95%   60.91%   -0.05%     
===========================================
  Files          192      192              
  Lines        14360    14385      +25     
===========================================
+ Hits          8753     8762       +9     
- Misses        5033     5041       +8     
- Partials       574      582       +8

[alessio]

Changes look straighforward. As long as tests pass, ACK from me

[alexanderbez]

Below are the notable changes @cwgoes @rigelrozanski

[rigelrozanski]

The issue I was concerned about today in the meeting does not apply (OK)

[rigelrozanski]

only looked at the core logic, needs some revisions

[alexanderbez]

Updated @rigelrozanski @cwgoes

[rigelrozanski]

approved pending one comment is addressed

[cwgoes]

I think this needs to be updated for #3828.

[codecov]

Codecov Report

Merging #3857 into develop will decrease coverage by 0.1%.
The diff coverage is 55.07%.

@@             Coverage Diff             @@
##           develop    #3857      +/-   ##
===========================================
- Coverage    60.45%   60.35%   -0.11%     
===========================================
  Files          196      196              
  Lines        14485    14526      +41     
===========================================
+ Hits          8757     8767      +10     
- Misses        5147     5174      +27     
- Partials       581      585       +4

[alexanderbez]

Updated pending entry. Left a followup response to @rigelrozanski.

[rigelrozanski]

per slack discussions:

func (v Validator) SharesFromTokensTruncate(amt sdk.Int) (sdk.Dec, sdk.Error) {
    if v.Tokens.IsZero() {
        return sdk.ZeroDec(), ErrInsufficientShares(DefaultCodespace)
    }

     return v.GetDelegatorShares().MulInt(amt).QuoTruncate(v.GetTokens()), nil
}
....

    // here TokensFromSharesRoundUp is a new function, where we round up in the final step 
    msgSharesRoundDown := validator.SharesFromTokensTruncate(msg.Tokens)
    delShares := del.GetShares()
    if msgSharesRoundDown.GT(delShares) {
        return ErrBadSharesAmount(k.Codespace()).Result()
    } 
    
    // now get the cap shares 
    shares = ...Capped shares amount (logic from redelegation) 

[cwgoes]

Failing simulation, looks like.

[rigelrozanski]

The changes make sense to me, however it would be good to have a test for this explicit edge case for using the cap of delegation shares. Also, simulations failing which must be rectified

[fedekunze]

update to MsgDelegate struct required

[alexanderbez]

@rigelrozanski Simulation passes now (not sure why it failed)
@fedekunze addressed your comments

wrt to unit tests, I'll be happy to add them but I'm not sure as to the best way to get fractional shares in the unit tests? Otherwise, there exist unit tests that already cover the over undelegation/redelegation cases.

[fedekunze]

LGTM

[alexanderbez]

@cwgoes mind taking a look at this prior to merging?

[cwgoes]

Mostly LGTM; a few possible minor adjustments.

ACK pending comments addressed.

[cwgoes]

... we can just delete this whole directory, we're not going to implement the Lamborghini model, unless there's a reason to keep it around. Certainly we don't need to update it, since it doesn't reflect any implementation.

[alexanderbez]

I think that's why it's in "attic". Should we just wipe this @rigelrozanski ?

[rigelrozanski]

I think it's a good idea to wipe... @ValarDragon wanted it in here for reference a while ago... either way I've added it to a new repo https://github.com/rigelrozanski/lamborghini-distribution

[cwgoes]

Sounds like the best of both worlds to me! We should delete it here then.

[cwgoes]

(oops - ACK pending comments addressed and conflict resolved)

[alexanderbez]

Updated comments @cwgoes and @rigelrozanski.