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

Updated account restrictions endpoints #28

Merged
merged 4 commits into from
Sep 30, 2019

Conversation

Vektrat
Copy link
Contributor

@Vektrat Vektrat commented Sep 27, 2019

@Vektrat Vektrat self-assigned this Sep 27, 2019
@dgarcia360
Copy link
Contributor

dgarcia360 commented Sep 29, 2019

I know that we are close to the deadline, but due to this change we might want to reconsider applying metadata's suggestions from @rg911 to achieve consistency between endpoints.

One of the reasons for not having separated metadata was due to the account restrictions endpoint structure:

Also, I don't see metadata as a single entity that works by its own. Taking into consideration similar features that depend on others (restrictions, multisig), we are creating endpoints under each main independent concept (e.g. /account/<:id>/restrictons, /account//multisig).

spec/openapi3.yaml Outdated Show resolved Hide resolved
spec/openapi3.yaml Outdated Show resolved Hide resolved
@dgarcia360 dgarcia360 self-requested a review September 29, 2019 07:55
@Vektrat
Copy link
Contributor Author

Vektrat commented Sep 29, 2019

I know that we are close to the deadline, but due to this change we might want to reconsider applying metadata's suggestions from @rg911 to achieve consistency between endpoints.

One of the reasons for not having separated metadata was due to the account restrictions endpoint structure:

Also, I don't see metadata as a single entity that works by its own. Taking into consideration similar features that depend on others (restrictions, multisig), we are creating endpoints under each main independent concept (e.g. /account/<:id>/restrictons, /account//multisig).

i don't really understand the suggestend endpoints here, maybe a list of endpoints + example result would help

but those are my concerns from what i think i understand: i don't see the benefits of this, what does metadata have to do with mosaic restrictions?
also, returning all the metadata given an accountId seems to me like it would be fetching a lot of data from the database and most of it would probably be useless to the client (i think fetching metadata by resource makes more sense than by accountId)

the only thing that makes sense to me, organizationally, is to have the endpoints such as /metadata/{type}/{other_params_conditional_to_type}/, which does not really reduce the number of endpoints

@dgarcia360
Copy link
Contributor

dgarcia360 commented Sep 30, 2019

@Vektrat Sorry for not being too specific, you are right. I was referring only to this change:

the only thing that makes sense to me, organizationally, is to have the endpoints such as /metadata/{type}/{other_params_conditional_to_type}/, which does not really reduce the number of endpoints

@rg911 rg911 merged commit b3ca9cb into master Sep 30, 2019
@dgarcia360 dgarcia360 deleted the task/updated-account-restrictions-endpoints branch November 11, 2019 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants