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

Flatten REST POST payload #3188

Closed
2 of 4 tasks
alessio opened this issue Dec 21, 2018 · 16 comments
Closed
2 of 4 tasks

Flatten REST POST payload #3188

alessio opened this issue Dec 21, 2018 · 16 comments
Labels
T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).

Comments

@alessio
Copy link
Contributor

alessio commented Dec 21, 2018

(sprung from #3179 (comment))

Presently, POST requests body embed a BaseReq nested object, e.g. POST tx/sign takes a JSON object like the following example:

{
  "tx": object,
  "append_sig": false,
  "base_req": {
    "name": "key",
    "password": "password",
    "chain_id": "gaia-9002"
  }
}

I'd propose to promote base_req fields and treat them as first-class citizens - this seems to make sense as those fields are required for the request to be successfully processed. That would simplify and flatten the expected data structure, which would then look like as follows:

{
  "tx": object,
  "append_sig": false,
  "name": "key",
  "password": "password",
  "chain_id": "gaia-9002"
}

Please discuss.

/CC @faboweb @alexanderbez @jackzampolin @fedekunze @sabau @cwgoes


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alessio alessio added S:proposed discussion T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). labels Dec 21, 2018
@alexanderbez
Copy link
Contributor

💯 % agree. The BaseReq should simply be embedded giving you a flat structure.

@faboweb
Copy link
Contributor

faboweb commented Dec 21, 2018

I actually think this structure makes sense as it decouples the content and the meta information.
I.e. for POST /bank/accounts/%s/transfers you split the amount and the signing info.

Not feeling too strongly about it. But would also avoid this refactor.

@alexanderbez
Copy link
Contributor

alexanderbez commented Dec 21, 2018

But there is no metadata fields persay...all the fields are primary/first-class-citizens imho.

@fedekunze
Copy link
Collaborator

I agree with this proposal, we can keep the base request struct for validation purposes and flatten the request body

@alexanderbez
Copy link
Contributor

@fedekunze precisely what we're suggesting!

@faboweb
Copy link
Contributor

faboweb commented Jan 6, 2019

Ok cool. I would just not do it because it doesn't deliver a clear value to me and is a breaking change.

@jackzampolin
Copy link
Member

@faboweb's concern is mine as well.

@alessio
Copy link
Contributor Author

alessio commented Jan 8, 2019

I think @faboweb got a point here, I can feel frontend's pain in making many changes for the sake of little to none value - can we at least change the inner object's name please? base_req means very little to me, maybe auth? Any better names anyone?

@faboweb
Copy link
Contributor

faboweb commented Jan 8, 2019

auth sounds good. Alternatives: meta_info, header, signing_info (which is not the case for generate_only)

@fedekunze
Copy link
Collaborator

what's the consensus on this proposal then ? I'm fine with leaving base_req as it is now.

@alexanderbez
Copy link
Contributor

alexanderbez commented Apr 5, 2019

I still agree that we should flatten it completely.

@alessio
Copy link
Contributor Author

alessio commented Apr 5, 2019

Up-to-date structure follows:

// BaseReq defines a structure that can be embedded in other request structures
// that all share common "base" fields.
type BaseReq struct {
	From          string       `json:"from"`
	Memo          string       `json:"memo"`
	ChainID       string       `json:"chain_id"`
	AccountNumber uint64       `json:"account_number"`
	Sequence      uint64       `json:"sequence"`
	Fees          sdk.Coins    `json:"fees"`
	GasPrices     sdk.DecCoins `json:"gas_prices"`
	Gas           string       `json:"gas"`
	GasAdjustment string       `json:"gas_adjustment"`
	Simulate      bool         `json:"simulate"`
}

auth and signing_info do not make much sense anymore as neither credentials nor signatures are carried with requests

@alessio
Copy link
Contributor Author

alessio commented Apr 5, 2019

meta sounds like a good name to me

@alexanderbez
Copy link
Contributor

Imho, we should flatten the type. meta doesn't make sense because it's not about the request -- it is the request.

@alessio
Copy link
Contributor Author

alessio commented Apr 5, 2019

Yes, fair enough.

@jackzampolin
Copy link
Member

Didn't we do this? Also we removed a large amount of data from the base_req. Going to go ahead and close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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