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

feat: rpc implementation [part 2/5] #2

Merged
merged 53 commits into from
Aug 8, 2024
Merged

Conversation

andreabadesso
Copy link
Collaborator

@andreabadesso andreabadesso commented Jun 13, 2024

Motivation

The idea of this library is to be used by the wallets or by any client wanting to accept the RPC calls defined in the RPC protocol

Some general notices:

  • The RPC design makes mixed use of camelCase and snake_case, we should standardize the params and update the design document, this will be done in the params validation PR (part3), which should be done before we're able to publish this library.
  • We should also validate the prompt responses, this will be done in the params validation PR (part3)

Acceptance Criteria

  • We should not validate parameters in this PR, it should be done in the part3 PR
  • We should have a better error handling mechanism, it should be done in the part3 PR
  • The library should be handle the following methods (with a few caveats):
    • htr_getAddress
      • the full_path parameter is not yet implemented, it should be implemented in the part4 PR
    • htr_getBalance
      • addressIndexes is not yet implemented, it should be implemented in the part4 PR
    • htr_getConnectedNetwork
      • This will not return the genesisHash defined in the RPC design as we don't yet have an
        API to fetch it from the fullnode nor a wallet-lib method to get it. This should be done in a
        later unplanned PR as it will involve changes in the fullnode as well as the lib.
    • htr_getUtxos
      • The implementation was copied from the headless, including the
        limitations, i.e. no pagination and potential memory issues when large
        wallets use this method. I think we can publish an initial version with it
        as it is, but create an issue to actually solve this as potentially more
        users will use this RPC (interacting with dApps) than headless users and
        the problem should be more evident on the mobile wallet with constrained
        memory.
    • htr_sendNanoContractTx
    • htr_signWithAddress

The following methods will be done in the part4 PR:

  • htr_createToken
  • htr_pushTxHex
  • htr_sendTx

The following method will be done in the part5 PR:

  • htr_getOperationStatus

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged
  • Make sure either the unit tests and/or the QA tests are capable of testing the new features
  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@andreabadesso andreabadesso self-assigned this Jun 13, 2024
@andreabadesso andreabadesso added the enhancement New feature or request label Jun 13, 2024
@andreabadesso andreabadesso changed the title feat: rpc implementation feat: rpc implementation [part 1/?] Jun 19, 2024
@andreabadesso andreabadesso changed the title feat: rpc implementation [part 1/?] feat: rpc implementation [part 1/4] Jun 19, 2024
@andreabadesso andreabadesso requested a review from tuliomir June 19, 2024 16:29
@andreabadesso andreabadesso changed the title feat: rpc implementation [part 1/4] feat: rpc implementation [part 2/4] Jun 19, 2024
@andreabadesso andreabadesso changed the title feat: rpc implementation [part 2/4] feat: rpc implementation [part 2/5] Jun 19, 2024
@andreabadesso andreabadesso force-pushed the feat/rpc-implementation branch 2 times, most recently from 996747f to a584b37 Compare June 20, 2024 23:38
Base automatically changed from chore/initial-structure to master June 21, 2024 00:00
@andreabadesso andreabadesso force-pushed the feat/rpc-implementation branch 2 times, most recently from 3ef1589 to aa16590 Compare July 12, 2024 13:41
@andreabadesso andreabadesso force-pushed the feat/rpc-implementation branch from aa16590 to e6d257c Compare July 12, 2024 13:44
Copy link

@tuliomir tuliomir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

I would only inform in the PR description itself that SendTx will be dealt with later, as there are many references to it in this PR and not having it implemented seems to be an oversight.

@andreabadesso andreabadesso merged commit 31b3f85 into master Aug 8, 2024
@andreabadesso andreabadesso deleted the feat/rpc-implementation branch August 8, 2024 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants