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

Fix compatibility with JSON RPC #24

Closed
davidyuk opened this issue May 20, 2019 · 4 comments · Fixed by #30
Closed

Fix compatibility with JSON RPC #24

davidyuk opened this issue May 20, 2019 · 4 comments · Fixed by #30

Comments

@davidyuk
Copy link
Member

JSON-RPC specification doesn't say anything about version field in the root of response/requests objects. If it is needed to store protocol version it should use params, result, or method name to be compatible with JSON RPC.

I have seen the usage of version field in AEX-2 and AEX-5.

@shekhar-shubhendu
Copy link
Contributor

@davidyuk we're trying to be as close to the state-channels JSON-RPC API which includes version field. you can find the example here.
The version field is as you guessed is needed to store protocol version.

@davidyuk
Copy link
Member Author

Are AEX-2 and AEX-5 somehow connected to state-channels? If it is not, I think we can make a better decision here.

@shekhar-shubhendu
Copy link
Contributor

it is not connected to but I would really like to keep the protocol version out of the method params as it does not makes sense to bind the version with the methods. If this breaks something we can find some other place to put it or we can remove it simply for now.

@shekhar-shubhendu
Copy link
Contributor

@davidyuk on another thought and after discussing it with @nduchak we can do it this way:

wallet sends the supported version in the connection request (wallet.request.connect) only and if version mismatch or unsupported then SDK/aepp throws an error.

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 a pull request may close this issue.

2 participants