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

Feature: Mesh API Implementation #1021

Merged
merged 125 commits into from
Nov 21, 2024
Merged

Feature: Mesh API Implementation #1021

merged 125 commits into from
Nov 21, 2024

Conversation

dhedey
Copy link
Contributor

@dhedey dhedey commented Nov 12, 2024

Summary

This PR implements the Mesh API for Radix.

Details

Data API

Endpoint Status
- /network/list Done
- /network/status Done
- /network/options Done
- /block Done
- /block/transaction Done, Withdraw and Deposit operations only
- /account/balance Done, no historical balance lookup
- /mempool Done
- /mempool/transaction Done, no operation estimation

Construction API

All endpoints have been implemented and only single signer/sender is supported.

Endpoint Status
- /construction/derive Done
- /construction/preprocess Done
- /construction/metadata Done
- /construction/payloads Done, Withdraw and Deposit operations only
- /construction/combine Done
- /construction/parse Done
- /construction/hash Done
- /construction/submit Done

Testing

We've added the mesh-cli test suites to CI.

@iamyulong iamyulong marked this pull request as ready for review November 15, 2024 17:07
Copy link
Contributor Author

@dhedey dhedey left a comment

Choose a reason for hiding this comment

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

Looking good! I'm half way through this review, so going to drop the comments I have so far -- all very minor.

I've yet to review the handlers and common.rs in full.

@@ -118,12 +119,14 @@ public final class RadixNodeModule extends AbstractModule {
private static final int DEFAULT_ENGINE_STATE_API_PORT = 3336;
private static final int DEFAULT_SYSTEM_API_PORT = 3334;
private static final int DEFAULT_PROMETHEUS_API_PORT = 3335;
private static final int DEFAULT_MESH_API_PORT = 3337;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a thought - we may need to talk to devops about getting this port hooked up in the babylon-nginx image.

docker/core.yml Show resolved Hide resolved
Copy link
Contributor Author

@dhedey dhedey left a comment

Choose a reason for hiding this comment

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

Some further comments. I can't approve as github thinks it's my PR!

Nearly all the comments are minor consistency / code location / naming improvements.

let fee_balance_change = fee_balance_changes
.entry(global_ancestor_address)
.or_insert_with(Decimal::zero);
*fee_balance_change = fee_balance_change.sub_or_panic(*paid_fee_amount_xrd);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume paid_fee_amount_xrd must be negative :D

Copy link
Contributor

@lrubasze lrubasze Nov 20, 2024

Choose a reason for hiding this comment

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

Not sure if I get this.
Whole method was copied from Core API

Copy link
Contributor Author

@dhedey dhedey Nov 21, 2024

Choose a reason for hiding this comment

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

Don't worry - this was just an unimportant observation.
I was a little surprised we were subtracting values, and expecting a positive balance coming out. But I assume paid_fee_amount_xrd must be negative.

ledger_header: &LedgerStateSummary,
) -> Result<models::BlockIdentifier, MappingError> {
to_mesh_api_block_identifier_from_state_version(ledger_header.state_version)
to_mesh_api_block_identifier_from_state_version(database, ledger_header.state_version)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

MINOR - In this case, we don't need to do a database read, because we have ledger_header.state_version and ledger_header.ledger_hashes :).

I'd perhaps expect a to_mesh_api_block_identifier(state_version: StateVersion, ledger_hashes: &LedgerHashes) which to_mesh_api_block_identifier_from_ledger_header can call directly, and to_mesh_api_block_identifier_from_state_version can call via the transaction_identifiers read.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wil fix it in next PR

@@ -1,3 +1,5 @@
api.mesh.enabled=$RADIXDLT_MESH_API_ENABLED
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we put this just above api.mesh.port?

Copy link
Contributor

Choose a reason for hiding this comment

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

Was really confused when you initially asked to put it in the first line 🤣

Copy link
Contributor Author

@dhedey dhedey left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM

Copy link
Contributor

@lrubasze lrubasze left a comment

Choose a reason for hiding this comment

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

Approve on @dhedey's behalf

@iamyulong iamyulong merged commit b8f3151 into develop Nov 21, 2024
23 checks passed
@iamyulong iamyulong deleted the feature/mesh branch November 21, 2024 21:42
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.

4 participants