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

coins exposed to clients should be map instead of array #4076

Closed
4 tasks
fedekunze opened this issue Apr 9, 2019 · 15 comments
Closed
4 tasks

coins exposed to clients should be map instead of array #4076

fedekunze opened this issue Apr 9, 2019 · 15 comments
Labels
C:x/bank S:proposed T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).

Comments

@fedekunze
Copy link
Collaborator

fedekunze commented Apr 9, 2019

Summary

Expose sdk.Coins as a map[string]sdk.Int instead of a slice of coins ([]sdk.Coin )

Problem Definition

currently sdk.Coins is defined as an array of sdk.Coin instead as a map (object), which makes it harder for clients to lookup a specific denomination since they have to filter the array

Proposal

sdk.Coin should remain as a it is rn, sdk.Coins should be refactored to a map[string]sdk.Int:

Proposed JSON

{
    "uatom": 10000000,
     "photon": 1500000
}

Current JSON

[
    {
        "denom": "uatom",
        "amount": 10000000
    },
   {
        "denom": "photon",
        "amount": 1500000
    }
]

I've experienced this pain point myself on Lunie as well as @sabau, @faboweb and @jbibla


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@fedekunze fedekunze added S:proposed core T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). C:x/bank labels Apr 9, 2019
@fedekunze fedekunze changed the title sdk.Coins should be map instead of array coins exposed to clients should be map instead of array Apr 9, 2019
@fedekunze fedekunze removed the core label Apr 9, 2019
@alexanderbez
Copy link
Contributor

alexanderbez commented Apr 9, 2019

I recall there was a reason we stuck with a slice instead of a map. For starters, maps are non-deterministic. So storing them as such may yield different application hashes. Not to mention, I doubt amino can even encode maps?

cc @ValarDragon

@fedekunze
Copy link
Collaborator Author

For starters, maps are non-deterministic.

yeah that's what @alessio told me

@alexanderbez
Copy link
Contributor

Great! Want to close this?

@fedekunze
Copy link
Collaborator Author

oh, no, I meant that I think we should still export the map to clients. Even tho if we keep the slice internally

@alexanderbez
Copy link
Contributor

alexanderbez commented Apr 9, 2019

You mean you want to implement Coins#ToMap? What would this provide? If you want to know if a denom exists or how much of a denom there is, use AmountOf (albeit it's slower).

@fedekunze
Copy link
Collaborator Author

to get the amount for a specific denom (eg uatom), clients need to implement a filter like the following:

const uatoms = coins.filter(coin => coin.denom === `uatom`)

@alexanderbez
Copy link
Contributor

I understand now. What you're suggesting is creating a custom JSON encoder/decoder. However, I don't see anything inherently wrong with needing to filter imho. Now coins won't be a true representation of their internal structure.

@ValarDragon
Copy link
Contributor

Surely theres a standard serializable golang hashmap?

@fedekunze
Copy link
Collaborator Author

I googled this and one of the top results was #553 lol

@fedekunze
Copy link
Collaborator Author

Also ref: #1273

@alexanderbez
Copy link
Contributor

If you want simple JSON map just for the clients, I think the following will suffice:

types/coin.go

// ----------------------------------------------------------------------------
// Encoding

type CoinJSON map[string]Int

func (coins Coins) MarshalJSON() ([]byte, error) {
	cj := make(CoinJSON)

	for _, c := range coins {
		cj[c.Denom] = c.Amount
	}

	return json.Marshal(cj)
}

func (coins *Coins) UnmarshalJSON(b []byte) error {
	cj := make(CoinJSON)

	if err := json.Unmarshal(b, &cj); err != nil {
		return err
	}

	for denom, amount := range cj {
		(*coins) = append((*coins), NewCoin(denom, amount))
	}

	coins.Sort()
	return nil
}

@sabau sabau added the REST label Apr 23, 2019
@sabau
Copy link
Contributor

sabau commented Apr 23, 2019

I think this will make life slightly easier for clients.
We are talking just about REST interfaces or also CLI (if someone want to build some sort of local client that fetch data from CLI)?
RPC seems out of context here right?

@fedekunze
Copy link
Collaborator Author

Just REST would suffice for the scope of this issue I think

@alexanderbez
Copy link
Contributor

With the code I showed above, it's as simple as that. You don't have to change anything else :)

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/bank S:proposed T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

No branches or pull requests

5 participants