feat(ICRC-Rosetta): updated rosetta to support icrc3 standard #2607
Conversation
| Ok(last_block_index_bytes) => last_block_index_bytes, | ||
| Err(_) => { | ||
| return Err(Icrc1AgentError::VerificationFailed(format!( | ||
| let last_block_index_leb128_encoded = lookup_leaf(&hash_tree, "last_block_index")?; |
There was a problem hiding this comment.
Does the new code still handel big endian byte encodings?
Can't the two encodings coincide? In which case the decision on what the encoding is should be based on the presence of tip_hash branch in the certificate.
| } | ||
|
|
||
| pub fn construct_hash_tree(&self) -> MixedHashTree { | ||
| pub fn construct_hash_tree(&self) -> HashTree { |
There was a problem hiding this comment.
What triggered this change? I couldn't find any test that verify that they produce the same value for this function, do we have one?
In general, the only test I found that checks the certificate is the //rs/tests/financial_integrations:icrc1_agent_test system test. Would it be possible to add a deterministic StateMachine test that verifies the certificate both with and without the feature flag, and ideally also with the MixedHashTree just to make sure that nothing changes with the move to HashTree?
There was a problem hiding this comment.
All the system tests for rosetta test the certificate as it is part of the block synchronization.
There was a problem hiding this comment.
I used HashTree since that is the official crate for ic-certification released on crates.io.
There was a problem hiding this comment.
All the system tests for rosetta test the certificate as it is part of the block synchronization.
I see, thanks for the pointer! I do find that a rather heavyweight approach, and a unit/integration test closer to the function itself would be useful. IIUC, the Rosetta system tests now also only test Rosetta with a ledger where the feature flag is enabled, but not with it disabled, and also only test with a client that understands both certificates, but not with one that only supports the legacy certificate.
There was a problem hiding this comment.
The icrc1 agent test tests the version of the icrc1 ledger where the feature flag is not set.
There was a problem hiding this comment.
The client understands both certificates. It is tested with both the icrc1 ledger with the feature flag (Rosetta tests) and without the feature flag (icrc1 agent test).
There was a problem hiding this comment.
The icrc1 agent test tests the version of the icrc1 ledger where the feature flag is not set.
Do you mean test_simple_start_of_synchronizing_blocks in rs/rosetta-api/icrc1/tests/integration_test_components/blocks_synchronizer/fetching_blocks_interval_test.rs? I couldn't find any tests in packages/icrc-ledger-agent. The aforementioned test also only calls the legacy get_data_certificate endpoint, and I couldn't find any code that actually calls icrc3_get_tip_certificate, which I feel should also be tested.
I find the current situation with only having Rosetta tests for this ledger feature to be a bit cumbersome and unintuitive, and I don't see a good reason for not having some simple tests under rs/ledger_suite/icrc1/ledger that call icrc3_get_tip_certificate with and without the feature flag.
There was a problem hiding this comment.
I meant: //rs/tests/financial_integrations:icrc1_agent_test.
I can add explicit tests for both endpoints too.
| } | ||
|
|
||
| pub fn construct_hash_tree(&self) -> MixedHashTree { | ||
| pub fn construct_hash_tree(&self) -> HashTree { |
There was a problem hiding this comment.
All the system tests for rosetta test the certificate as it is part of the block synchronization.
I see, thanks for the pointer! I do find that a rather heavyweight approach, and a unit/integration test closer to the function itself would be useful. IIUC, the Rosetta system tests now also only test Rosetta with a ledger where the feature flag is enabled, but not with it disabled, and also only test with a client that understands both certificates, but not with one that only supports the legacy certificate.
Co-authored-by: Mathias Björkqvist <[email protected]>
| match block_hash.clone().try_into() { | ||
| Ok(last_block_hash) => Ok(last_block_hash), | ||
| Err(_) => { | ||
| return Err(Icrc1AgentError::VerificationFailed(format!( | ||
| "DataCertificate last_block_hash bytes: {}, cannot be decoded as last_block_hash", | ||
| hex::encode(last_block_hash_vec) | ||
| ))) | ||
| Err(Icrc1AgentError::VerificationFailed(format!( | ||
| "DataCertificate last_block_hash bytes: {}, cannot be decoded as last_block_hash", | ||
| hex::encode(block_hash) | ||
| ))) |
There was a problem hiding this comment.
I believe this could be simplified as
block_hash.clone().try_into().or(Err(Icrc1AgentError::...
This MR proposes the following changes:
latest_block_hashinstead oftip_hashin the ICRC3 certificatelatest_block_indexin the ICRC3 certificate