refactor(iota-core): Add more comments for `ReadApi::try_get_object_before_version` #3844

bingyanglin · 2024-11-01T08:52:38Z

Description of change

Added more comments to explain why we still keep it.

Links to any relevant issues

Part of #2092

Type of change

Enhancement

How the change has been tested

Ran the local network with RUST_LOG=info cargo run --release --bin iota start --force-regenesis --with-faucet

Change checklist

Tick the boxes that are relevant to your changes, and delete any items that are not.

I have followed the contribution guidelines for this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
I have checked that new and existing unit tests pass locally with my changes

kodemartin

Could you please elaborate on why this is deprecated?

bingyanglin · 2024-11-01T09:25:51Z

Could you please elaborate on why this is deprecated?

Yeah this is a good question.
From my side I saw there is a deprecated mark for try_get_object_before_version.

    #[method(name = "tryGetObjectBeforeVersion", deprecated = "true")]
    async fn try_get_object_before_version(

This was listed in #2092 and identified by @thibault-martinez, maybe Thibault can elaborate better?

thibault-martinez · 2024-11-01T09:29:01Z

This was listed in #2092 and identified by @thibault-martinez, maybe Thibault can elaborate better?

I'm afraid I can't expand on the why... I just listed everything flagged as deprecated for further investigation.

bingyanglin · 2024-11-01T09:36:29Z

@kodemartin @thibault-martinez

Based on the original SUI PR here for the deprecated mark, it says this is only used by sui replay tool. I think it might be fine to keep it for now.

Would like to know your opinions, thanks.

kodemartin · 2024-11-01T10:08:54Z

Thanks for your answers @bingyanglin @thibault-martinez! I missed the deprecated flag.

I see that the implementation in iota-json-rpc uses internally the AuthorityState::find_object_lt_or_eq_version method that is delegated to

iota/crates/iota-core/src/authority/authority_store.rs

Lines 1421 to 1426 in 1c55189

    
               /// Return the object with version less then or eq to the provided seq 
        
               /// number. This is used by indexer to find the correct version of 
        
               /// dynamic field child object. We do not store the version of the child 
        
               /// object, but because of lamport timestamp, we know the child must 
        
               /// have version number less then or eq to the parent. 
        
               pub fn find_object_lt_or_eq_version(

Given that it seems to be some underlying utility, and that the calling method is not deprecated I agree with @bingyanglin that we should keep it as well.

thibault-martinez · 2024-11-01T10:55:49Z

Given that it seems to be some underlying utility, and that the calling method is not deprecated I agree with @bingyanglin that we should keep it as well.

Definitely fine by me! Maybe I would transform this PR into leaving a small comment about why it's deprecated and kept, but it's also fine without.

bingyanglin · 2024-11-01T11:38:21Z

Given that it seems to be some underlying utility, and that the calling method is not deprecated I agree with @bingyanglin that we should keep it as well.

Definitely fine by me! Maybe I would transform this PR into leaving a small comment about why it's deprecated and kept, but it's also fine without.

Thanks @kodemartin and @thibault-martinez for the reply. I added more comments in ca140b3 and keep this endpoint.

crates/iota-json-rpc-api/src/read.rs

Co-authored-by: Thibault Martinez <[email protected]>

kodemartin · 2024-11-01T12:07:39Z

Why not remove the deprecated flag altogether? @bingyanglin @thibault-martinez

bingyanglin · 2024-11-01T12:16:26Z

Why not remove the deprecated flag altogether? @bingyanglin @thibault-martinez

Agreed. Removed the flag at 6dd98c8.

kodemartin

lgtm 🌸

thibault-martinez

It was introduced as already deprecated to avoid people calling it directly, if we think that's not an issue then LGTM!

…re-version

crates/iota-json-rpc-api/src/read.rs

muXxer · 2024-11-01T13:58:48Z

crates/iota-json-rpc-api/src/read.rs

+    // underlying utility, e.g., `RemoteFetcher::get_child_object` uses
+    // `try_get_object_before_version` to get the object with the versions <=
+    // the given version. Thus we keep this endpoint for now.
+    #[method(name = "tryGetObjectBeforeVersion")]


I think we should keep the deprecated flag here.
As far as I understand, having the flag causes it to not be exposed in the generated spec file.
Since it should be only for internal usage, I guess the comment is enough to not remove the function, even though it is marked as deprecated (I guess they did it for the same reason as explained).

Let's keep the flag to make it clearer. Added the flag at 5c480e4.

Also enhanced the comment at 8abb3b9.

…re-version

bingyanglin added the node Issues related to the Core Node team label Nov 1, 2024

bingyanglin self-assigned this Nov 1, 2024

bingyanglin requested review from a team as code owners November 1, 2024 08:52

bingyanglin mentioned this pull request Nov 1, 2024

List of deprecated code #2092

Closed

18 tasks

kodemartin reviewed Nov 1, 2024

View reviewed changes

Add more comments for try_get_object_before_version

ca140b3

bingyanglin force-pushed the core-node/feat/remove-try-get-object-before-version branch from a069036 to ca140b3 Compare November 1, 2024 11:36

Remove unnecessary parenthesis

57a188b

thibault-martinez reviewed Nov 1, 2024

View reviewed changes

crates/iota-json-rpc-api/src/read.rs Outdated Show resolved Hide resolved

bingyanglin changed the title ~~refactor(iota-core): Remove ReadApi::try_get_object_before_version~~ refactor(iota-core): Add more comments for ReadApi::try_get_object_before_version Nov 1, 2024

Update crates/iota-json-rpc-api/src/read.rs

858f9e8

Co-authored-by: Thibault Martinez <[email protected]>

Remove deprecated mark

6dd98c8

kodemartin approved these changes Nov 1, 2024

View reviewed changes

thibault-martinez approved these changes Nov 1, 2024

View reviewed changes

Merge branch 'develop' into core-node/feat/remove-try-get-object-befo…

de65616

…re-version

DaughterOfMars reviewed Nov 1, 2024

View reviewed changes

crates/iota-json-rpc-api/src/read.rs Outdated Show resolved Hide resolved

Use code comments

e0c4ec4

muXxer requested changes Nov 1, 2024

View reviewed changes

bingyanglin and others added 5 commits November 1, 2024 22:11

Add the deprecated flag back

5c480e4

Merge branch 'develop' into core-node/feat/remove-try-get-object-befo…

d11f504

…re-version

Merge branch 'develop' into core-node/feat/remove-try-get-object-befo…

957cea9

…re-version

Enhance the code comment

8abb3b9

Merge branch 'develop' into core-node/feat/remove-try-get-object-befo…

8db8c39

…re-version

muXxer approved these changes Nov 4, 2024

View reviewed changes

muXxer merged commit 02f1459 into develop Nov 4, 2024
38 of 40 checks passed

muXxer deleted the core-node/feat/remove-try-get-object-before-version branch November 4, 2024 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(iota-core): Add more comments for `ReadApi::try_get_object_before_version` #3844

refactor(iota-core): Add more comments for `ReadApi::try_get_object_before_version` #3844

bingyanglin commented Nov 1, 2024 •

edited

Loading

kodemartin left a comment

bingyanglin commented Nov 1, 2024 •

edited

Loading

thibault-martinez commented Nov 1, 2024

bingyanglin commented Nov 1, 2024 •

edited

Loading

kodemartin commented Nov 1, 2024

thibault-martinez commented Nov 1, 2024

bingyanglin commented Nov 1, 2024

kodemartin commented Nov 1, 2024

bingyanglin commented Nov 1, 2024

kodemartin left a comment

thibault-martinez left a comment

muXxer Nov 1, 2024

bingyanglin Nov 1, 2024 •

edited

Loading

bingyanglin Nov 4, 2024

refactor(iota-core): Add more comments for ReadApi::try_get_object_before_version #3844

refactor(iota-core): Add more comments for ReadApi::try_get_object_before_version #3844

Conversation

bingyanglin commented Nov 1, 2024 • edited Loading

Description of change

Links to any relevant issues

Type of change

How the change has been tested

Change checklist

kodemartin left a comment

Choose a reason for hiding this comment

bingyanglin commented Nov 1, 2024 • edited Loading

thibault-martinez commented Nov 1, 2024

bingyanglin commented Nov 1, 2024 • edited Loading

kodemartin commented Nov 1, 2024

thibault-martinez commented Nov 1, 2024

bingyanglin commented Nov 1, 2024

kodemartin commented Nov 1, 2024

bingyanglin commented Nov 1, 2024

kodemartin left a comment

Choose a reason for hiding this comment

thibault-martinez left a comment

Choose a reason for hiding this comment

muXxer Nov 1, 2024

Choose a reason for hiding this comment

bingyanglin Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

bingyanglin Nov 4, 2024

Choose a reason for hiding this comment

refactor(iota-core): Add more comments for `ReadApi::try_get_object_before_version` #3844

refactor(iota-core): Add more comments for `ReadApi::try_get_object_before_version` #3844

bingyanglin commented Nov 1, 2024 •

edited

Loading

bingyanglin commented Nov 1, 2024 •

edited

Loading

bingyanglin commented Nov 1, 2024 •

edited

Loading

bingyanglin Nov 1, 2024 •

edited

Loading