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/types 32 bit support #334

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

rpc/types 32 bit support #334

wants to merge 2 commits into from

Conversation

Boog900
Copy link
Member

@Boog900 Boog900 commented Nov 1, 2024

What

Makes changes to some crates to support 32 bit systems for rpc/types

Why

WASM

@github-actions github-actions bot added A-dependency Related to dependencies, or changes to a Cargo.{toml,lock} file. A-workspace Changes to a root workspace file or general repo file. A-net Related to networking. A-rpc Related to RPC. labels Nov 1, 2024
@spirobel
Copy link

spirobel commented Nov 3, 2024

lgtm 👍

I managed to construct a request object for get_blocks.bin with this branch.

some observations regarding the types:
Afterwards i passed the request object to typescript and used it in the body of the fetch request. The response was then passed back to rust and parsed into a GetBlocksResponse object.

First I used GetBlocksRequest::default() to construct the request which meant the start_height was set to 0.

As a result the finish function gave me: "Required field was not found!"

fn finish(self) -> error::Result<GetBlocksResponse> {

This is because there are no Blocks in this response.
Afterwards I set the start_height to 100. So the response contained blocks.
The result was still: "Required field was not found!"

This time this was because of

  let pool_info_extent = self.pool_info_extent.ok_or(ELSE)?; 

After I manually inserted

 let pool_info_extent = PoolInfoExtent::None;

the Response was parsed successfully.

Suggestions:

  1. Empty blocks should be a valid GetBlocksResponse object.
  2. pool_info_extent should be set to None instead of throwing an error
  3. the error message should indicate which field is missing.

EDIT:
same applies to daemon_time. Both of these values get turned into None https://github.com/monero-project/monero/blob/893916ad091a92e765ce3241b94e706ad012b62a/src/rpc/core_rpc_server_commands_defs.h#L265 (macro reference https://github.com/monero-project/monero/blob/893916ad091a92e765ce3241b94e706ad012b62a/contrib/epee/include/serialization/keyvalue_serialization.h#L90)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependency Related to dependencies, or changes to a Cargo.{toml,lock} file. A-net Related to networking. A-rpc Related to RPC. A-workspace Changes to a root workspace file or general repo file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants