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

RPC client refactoring plan #2597

Closed
roman-khimov opened this issue Jul 13, 2022 · 4 comments
Closed

RPC client refactoring plan #2597

roman-khimov opened this issue Jul 13, 2022 · 4 comments
Labels
discussion Open discussion of some problem rpc RPC server and client

Comments

@roman-khimov
Copy link
Member

#2585 and #2586 solved the problem of excessive client dependencies, now we can move on to other topics. We already have #1967, but that's not the only problem we have, even though it sets the main direction of RPC client evolution. But we have other problems as well:

  1. Packaging.
    Unfortunately, client and server are not a very good package names. We also have data types split between request, response and response/result. All of this was done quite a long time ago and can be improved to simplify using the client. But it's controversial at the same time, because it's a breaking change.
  2. smartcontract.Parameter type
    While it's very natural for the RPC protocol we have is not very usable without an ability to convert convert from/to regular Go types (like toStackParameter in NeoFS).
  3. Too many things happening in the Client
    Explained in Wrap contracts into another structure in RPC client #1967, we have both simple API requests there as well as higher-order functions like NEP17Symbol or MultiTransferNEP17.
  4. Missing wrappers for native contracts
    While NEP-17 functions are present, one can't easily do vote in NEO contract, for example.
  5. Contract-specific clients can't be created easily
    Backend code mostly wants to work with some particular contract and while it can implement NEP-17 or other standard it's also very likely to have some task-specific methods that aren't easily accessible currently.
  6. Missing some wait-loop helpers
    State-changing requests normally produce a transaction and the client then needs to wait for the result. While the best way to do that is via subscriptions, it can also do polling and currently it's out of client's scope.
  7. State-changing functions that create transactions internally don't return ValidUntilBlock.
    Which means that the client doesn't know how long to wait for the transaction to be accepted (or not).
  8. Some methods have "create tx, but don't send" methods, some don't
    There has to be some uniformity there, proper client needs both.
  9. Wrappers don't usually provide a way to test-exec something
    Any state-changing method should be available for test execution as well as creation of a complete transaction and its transmission.
  10. Wrappers don't provide any easy way to perform historic calls.
    Again, any method can be invoked with some old state and this should be possible to do easily without moving a layer below (calling generic InvokeFunctionAtHeight or similar).

What I'd like to do is to concentrate first on the functionality we have, not expanding it in the first phase. At the same time breaking changes are better be done eariler, therefore the plan that I have in mind is:

  1. Rename rpc/client to rpc/rpcclient, rpc/server to rpc/rpcserver and merge request/response/result into neojson package. So it'll be like neojson.Request, neojson.Response, neojson.Invocation. I'm interested in opinions on this though.
  2. Move script creation routines out of the client to the smartcontract package. It's there exactly for this purpose.
  3. Provide smartcontract.Parameter helpers and add client APIs that will use bare Go types for invocations.
  4. Strip the client from the methods not directly related to the original RPCs. Move nep11 and nep17 functions to the respective packages that are to be initialized with the Client and token contract hash. Move functions for native contracts we have into packages of their own reusing nep17 where possible. This will affect all clients, unfortunately. I'd like to keep old APIs available for at least one release along with the new ones, but I'm not sure it'll be possible to do this due to potential package import cycles.
  5. Make APIs consistent wrt the abilities they provide for different classes of functions. It should be possible to create a script for some invocation, create a transaction for it and send it. Or do all of this with a single call.
  6. Extend native contract packages to cover all functions needed. This can be done in phases, we don't need all of the native contracts at once, but we can start with NEO and Oracle and then extend the set when needed (Notary is a notable target as well).
  7. Provide test-exec abilities to all wrapped contract-specific calls.
    Either a set of new methods (nep17.Transfer vs nep17.TestTransfer) or some interface implementation (nep17.Transfer but with modified underlying client or setting). But it's likely to be the first one as test executions provide a lot of additional data that might be useful for the client in various circumstances.
  8. Provide historic calls for all wrapped calls.
    Via an interfaced lower-layer client that will be initialized with some state definition, like
    c := client.New(...)
    hi := c.HistoricInvoker(hash/block)
    hnep17 := nep17.New(hi, token_hash) // Instead of the usual `nep17.New(c, token_hash)`
    out := hnep17.Transfer(...)
    
  9. Add wait loops.
  10. Only after all of this we can think of extending compiler (I think bindings data can be used as the basis) to generate additional data which then will be used to automatically provide wrappers for contracts.

Unfortunately, there will be a lot of breaking changes, so I'm interested in opinions on how can we make this transition period less stressful for people. And in general, what can we do more or less.

@roman-khimov roman-khimov added rpc RPC server and client discussion Open discussion of some problem labels Jul 13, 2022
@fyrchik
Copy link
Contributor

fyrchik commented Jul 13, 2022

So it'll be like neojson.Request, neojson.Response, neojson.Invocation. I'm interested in opinions on this though.

I think neorpc or rpcmessage is less ambiguious than neojson. It is also more similar to rpcclient/rpcserver.

Provide smartcontract.Parameter helpers and add client APIs that will use bare Go types for invocations.

We already have something similar in neofs-adm, where we use emit machinery to handle conversions https://github.com/nspcc-dev/neofs-node/blob/f4a3fc5f1162c2d3dbabab41726ddbde28c952ec/cmd/neofs-adm/internal/modules/morph/local_client.go#L327 . Not sure it is suitable for rpcclient though, as there is a separate invokefunction RPC.

However, it would be nice to have Emit for smartcontract.Parameter too.

@roman-khimov
Copy link
Member Author

roman-khimov commented Aug 26, 2022

We're at 4-6 at the moment, 4 is not yet completed (notary, nns, tokeninfo), yet at the same time we already have a nice contract coverage for 6 and at least partially established APIs for 5 (Method (that sends transaction), MethodTransaction (signs, but not sends), MethodUnsigned (not signed transaction returned)).

And yet we have 8 done for reader calls.

@roman-khimov
Copy link
Member Author

roman-khimov commented Sep 9, 2022

We're done up to number 6 now. We don't have a wrapper for StdLib or CryptoLib contracts, but I doubt these wrappers can be useful, these contracts are just libraries to be used by other contracts. We also don't have one for the Ledger contract, but all of what it does is accessible via regular RPC API, no point in wrapping it. All the other contracts are completely available.

As for 7. We have the concept of TransactionCheckerModifier in the Actor and it probably solves the problem. It's easy to do any kind of checks in this callback before transaction is created/sent. So I this it's covered as well, but @alexvanin and @fyrchik please tell me if I'm wrong.

We also have full historic abilities in Invoker for 8 (and any reader API building on top of it). Actor does not have it, but it's made for state-changing calls, so it'd be strange for it to try creating transaction that is run against the old state. If any test-calls are needed they're available via Invoker. Seems to be OK to me.

So we can move on to canonical wait loops of 9 and then a separate issue for 10.

@roman-khimov
Copy link
Member Author

I've created #2704 and #2705 to simplify tracking these specific things that we still have left. This issue can be closed, everything else is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Open discussion of some problem rpc RPC server and client
Projects
None yet
Development

No branches or pull requests

2 participants