Add binary codec and transaction models for MPT#131
Conversation
baf2e82 to
841449f
Compare
841449f to
7925f94
Compare
7925f94 to
d136241
Compare
2792500 to
84f099a
Compare
|
/ai-review |
|
@e-desouza can you add integration tests for all transactions? |
|
Added in The authorize/set/destroy tests all need a real issuance to work against, so I added a |
380da30 to
44cc257
Compare
… #3270) (#291) ## Summary The `rippled` binary was renamed to `xrpld` upstream, and the `rippleci/rippled` image stopped receiving updates. Our integration tests across every open PR started failing because the published `develop` image exited before becoming healthy (`Connection refused` on `localhost:5005`, **0 passed / 41 failed**). This PR mirrors the upstream fix in xrpl.js: [XRPLF/xrpl.js#3270](XRPLF/xrpl.js#3270). Switching to `rippleci/xrpld:develop` is the **actual root-cause fix** rather than pinning an old digest of the deprecated image. ## Changes `.github/workflows/integration_test.yml`: - `RIPPLED_DOCKER_IMAGE` -> `XRPLD_DOCKER_IMAGE: rippleci/xrpld:develop`. - `docker run` simplified to `${IMAGE} --standalone` (the `xrpld` image handles `mkdir` + launch internally; no more `bash -c "mkdir -p /var/lib/rippled/db/ && rippled -a"` wrapper). - Volume mount changed from `/etc/opt/ripple/` to `/etc/opt/xrpld/`. - Container name: `rippled-service` -> `xrpld-service`. - Removed the docker `--health-cmd` (which shelled out to the renamed `rippled` CLI and always failed) in favour of a direct JSON-RPC poll against `http://localhost:5005/`. - Always dump container logs on the stop step for post-mortem visibility. `.ci-config/rippled.cfg` -> `.ci-config/xrpld.cfg`: - `path=/var/lib/rippled/db/nudb` -> `path=/var/lib/xrpld/db/nudb`. - `[database_path] /var/lib/rippled/db` -> `/var/lib/xrpld/db`. - `[debug_logfile] /var/log/rippled/debug.log` -> `/var/log/xrpld/debug.log`. ## Verification Validated on throwaway PR #292 (now closed): **Integration Test green in 2m53s** on this exact workflow. Unit tests, Build & Lint, Quality Check also pass. ## Related follow-up The 7 in-flight PRs (#130, #131, #151, #153, #156, #157, #158) currently carry a stopgap commit pinning `rippleci/rippled:develop` to a specific digest. After this PR merges to `main`, those branches should: 1. Rebase on `main` to pick up the xrpld switch, or 2. Cherry-pick this commit and drop the stopgap digest pin. ## Test plan - [x] Validated end-to-end on PR #292 - [x] Build & Lint, Unit Test, Integration Test, Quality Check all pass - [ ] Merge and confirm subsequent PRs inherit the fix without manual cherry-pick ## Credit Approach lifted from @ckeshava's [xrpl.js#3270](XRPLF/xrpl.js#3270).
bef984f to
091ddde
Compare
|
@copilot resolve the merge conflicts in this pull request |
Resolved by merging |
| @@ -276,3 +276,47 @@ where | |||
| ); | |||
| ledger_accept().await; | |||
There was a problem hiding this comment.
Thanks for adding the wait_for_ledger_close_time method. However, this change is not sufficient. The above assert statement is still working on a temporary transaction-status result.
The correct way to validate the status of a transaction would be to invoke the Tx RPC call on the next validated ledger. This will provide the authoritative grouth-truth info from the blockchain-state.
For example, here is the pattern used by xrpl.js integ-tests to verify that the submitted transaction was in-fact processed correctly. Specifically, we need a Rust-equivalent of the verifySubmittedTransaction method."
After deeper inspection, I acknowledge that this is missing in the integration-test harness of the xrpl-py SDK. This is something that I will raise with the team and prioritize for future work of xrpl-py SDK.
| // Faucet errors | ||
| "Funding request timed out", | ||
| "faucet", | ||
| // WS result-type mismatch from unexpected push messages (local rippled) | ||
| "Unexpected result type", |
There was a problem hiding this comment.
IMO we should not have this type of file because its a code-smell. The tests should be as stringent as possible.
In the current integ-test harness, we are liberally allowing all of these errors which mask our understanding.
None of the other SDKs have this type of "allow-list" of potential errors for a correctly structured transaction.
There was a problem hiding this comment.
Ideally, we should have granular numeric/enum error codes, but it seems we lose fidelity when we map real errors to the existing error codes. Will analyze and see if this is something we can improve in the ecosystem overall.
Will try to fix the above to map to error codes/something more robust in the meantime.
There was a problem hiding this comment.
This is addressed by the typed error cleanup in 3a93900 and bd5e337. We no longer rely on a broad string allow-list for branch-touched network/RPC paths: transport failures classify through the centralized XRPLNetworkErrorKind helpers in src/asynch/clients/*/exceptions.rs, and server RPC failures classify through reusable XRPLRpcError in src/models/results/mod.rs.
| // Skip only on infrastructure failures after connect (timeout, reset, HTTP layer). | ||
| // Semantic XRPL errors (actNotFound, malformedTx, etc.) propagate as failures. | ||
| if msg.contains("timed out") | ||
| || msg.contains("timeout") | ||
| || msg.contains("Connection reset") | ||
| || msg.contains("Connection refused") | ||
| || msg.contains("reqwest") | ||
| || msg.contains("hyper") | ||
| || msg.contains("EmptyResponse") | ||
| { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
IMO we should not liberally allow these errors. This test-structure can hide regression errors.
For example, in the future, if the WebSocket connection's port is updated to 51232, then these tests will pass although the rippled server returns "Connection Refused" on port 51233.
I don't see the benefits of allowing these errors to pass through.
There was a problem hiding this comment.
This is addressed by the same typed error cleanup in 3a93900 and bd5e337. The broad pass-through behavior was removed for the branch-touched tests; transport errors are now classified via centralized XRPLNetworkErrorKind instead of brittle string matching, and server-side RPC errors use XRPLRpcError (including is_server_network_state) where relevant.
| #[test] | ||
| fn test_maximum_amount_zero_is_ok() { | ||
| let txn = MPTokenIssuanceCreate { | ||
| common_fields: CommonFields { | ||
| account: "rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B".into(), | ||
| transaction_type: TransactionType::MPTokenIssuanceCreate, | ||
| ..Default::default() | ||
| }, | ||
| maximum_amount: Some("0".into()), | ||
| ..Default::default() | ||
| }; | ||
| assert!(txn.validate().is_ok()); | ||
| } |
There was a problem hiding this comment.
This unit-test diverges from the rippled behavior. In fact, rippled rejects these types of transactions with temMALFORMED error.
There was a problem hiding this comment.
Addressed in 6957139. MaximumAmount validation now matches xrpld: zero is rejected, values above i64::MAX are rejected, and the max accepted value is covered by a positive test.
| #[test] | ||
| fn test_transfer_fee_zero_without_flag_error() { | ||
| // Some(0) without TfMPTCanTransfer should still be rejected — the guard | ||
| // fires on is_some() regardless of value, matching rippled behaviour. | ||
| let txn = MPTokenIssuanceCreate { | ||
| common_fields: CommonFields { | ||
| account: "rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B".into(), | ||
| transaction_type: TransactionType::MPTokenIssuanceCreate, | ||
| ..Default::default() | ||
| }, | ||
| transfer_fee: Some(0), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| assert!(txn.validate().is_err()); | ||
| } |
There was a problem hiding this comment.
Hello, did you mean to demonstrate a unit-test where non-zero transfer fee + no-flag will throw an error? It is not obvious that this case actually throws an error with rippled.
Here is the validation source code of rippled. Only non-zero transfer-fees + no-flag are rejected here.
There was a problem hiding this comment.
Addressed in 6957139. The validation now rejects non-zero TransferFee without tfMPTCanTransfer, while TransferFee: 0 without the flag is allowed. Tests cover both cases.
… #3270) (XRPLF#291) ## Summary The `rippled` binary was renamed to `xrpld` upstream, and the `rippleci/rippled` image stopped receiving updates. Our integration tests across every open PR started failing because the published `develop` image exited before becoming healthy (`Connection refused` on `localhost:5005`, **0 passed / 41 failed**). This PR mirrors the upstream fix in xrpl.js: [XRPLF/xrpl.js#3270](XRPLF/xrpl.js#3270). Switching to `rippleci/xrpld:develop` is the **actual root-cause fix** rather than pinning an old digest of the deprecated image. ## Changes `.github/workflows/integration_test.yml`: - `RIPPLED_DOCKER_IMAGE` -> `XRPLD_DOCKER_IMAGE: rippleci/xrpld:develop`. - `docker run` simplified to `${IMAGE} --standalone` (the `xrpld` image handles `mkdir` + launch internally; no more `bash -c "mkdir -p /var/lib/rippled/db/ && rippled -a"` wrapper). - Volume mount changed from `/etc/opt/ripple/` to `/etc/opt/xrpld/`. - Container name: `rippled-service` -> `xrpld-service`. - Removed the docker `--health-cmd` (which shelled out to the renamed `rippled` CLI and always failed) in favour of a direct JSON-RPC poll against `http://localhost:5005/`. - Always dump container logs on the stop step for post-mortem visibility. `.ci-config/rippled.cfg` -> `.ci-config/xrpld.cfg`: - `path=/var/lib/rippled/db/nudb` -> `path=/var/lib/xrpld/db/nudb`. - `[database_path] /var/lib/rippled/db` -> `/var/lib/xrpld/db`. - `[debug_logfile] /var/log/rippled/debug.log` -> `/var/log/xrpld/debug.log`. ## Verification Validated on throwaway PR XRPLF#292 (now closed): **Integration Test green in 2m53s** on this exact workflow. Unit tests, Build & Lint, Quality Check also pass. ## Related follow-up The 7 in-flight PRs (XRPLF#130, XRPLF#131, XRPLF#151, XRPLF#153, XRPLF#156, XRPLF#157, XRPLF#158) currently carry a stopgap commit pinning `rippleci/rippled:develop` to a specific digest. After this PR merges to `main`, those branches should: 1. Rebase on `main` to pick up the xrpld switch, or 2. Cherry-pick this commit and drop the stopgap digest pin. ## Test plan - [x] Validated end-to-end on PR XRPLF#292 - [x] Build & Lint, Unit Test, Integration Test, Quality Check all pass - [ ] Merge and confirm subsequent PRs inherit the fix without manual cherry-pick ## Credit Approach lifted from @ckeshava's [xrpl.js#3270](XRPLF/xrpl.js#3270).
3807217 to
a88d217
Compare
a02b59f to
c502be0
Compare
c502be0 to
62965cd
Compare
|
Split the prerequisite plumbing into stacked PRs so this PR can stay focused on MPT:
I force-pushed #131 with |
Summary
Adds XLS-0033 Multi-Purpose Token (MPT) support to
xrpl-rust, including typed models, binary codec support, request/result handling, and integration coverage against localxrpld/Clio services.MPT Support
MPTAmountMPTCurrencyAmount::MPTAmountCurrency::MPTCurrencyMPTokenIssuanceCreateMPTokenIssuanceDestroyMPTokenIssuanceSetMPTokenAuthorizeMPTokenMPTokenIssuanceaccount_objectsMPT filters formpt_issuanceandmptoken.Correctness / Review Follow-ups
xrpldbehavior:tfMPTCanTransfer.AmountandCurrencydeserialization to avoid ambiguous MPT/issued-currency fallback.XRPLRpcErrormapping where touched.no_stdcompatibility for model/helper paths.Integration Coverage
Adds end-to-end MPT coverage for:
tfMPTCanTransferis absent.Validation
Validated locally against Podman
xrpld/ Clio services:cargo fmt --checkgit diff --checkcargo test --features integration --lib -- --test-threads=1cargo test --features integration --test integration_test -- --test-threads=1cargo test --features integration --test cli_integration -- --test-threads=1cargo test --no-default-features --features models --lib -- --test-threads=1cargo test --no-default-features --features "models,wallet,helpers,json-rpc,embassy-rt" --lib -- --test-threads=1cargo test --no-default-features --features "models,wallet,helpers,websocket,embassy-rt" --lib -- --test-threads=1cargo check --no-default-features --features "models,wallet,helpers,json-rpc,websocket,embassy-rt"