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

Bitswap: rename deleteXX to removeXX, add tag to IPC #16072

Merged
merged 9 commits into from
Oct 16, 2024

Conversation

svv232
Copy link
Member

@svv232 svv232 commented Sep 18, 2024

This PR renames a number of methods with names deleteXX to removeXX.

It also adds a functions create_download_resource_push_message and create_remove_resource_push_message that will later be used for organization of IPC between Ocaml's bit-catchup algrotihm code and Libp2p's functions exposing Biytswap functionality.

@svv232 svv232 requested a review from a team as a code owner September 18, 2024 01:59
@svv232
Copy link
Member Author

svv232 commented Sep 18, 2024

!ci-build-me

@@ -332,7 +332,7 @@ struct Libp2pHelperInterface {
result @1 :ValidationResult;
}

struct DeleteResource {
struct RemoveResource {
Copy link
Member

Choose a reason for hiding this comment

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

A bit odd that we use tag below but not here, do you have an answer why?

Copy link
Member Author

Choose a reason for hiding this comment

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

tag is a bit opaque I agree, it is mainly used in the mina net 2 library. The purpose seems to be to detect the type of body to sync. For example you may have a different action enabled for downloading epoch ledger vs block.

Copy link
Member

Choose a reason for hiding this comment

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

UPD tag is part of encoded data, and root block id is hash of something derived from that data.

I.e. tag is sometimes provided to API for purposes of differentiating which function to use for the data. But for removal it isn't needed, data is perfectly referenced by the root block id alone.

@@ -52,14 +52,14 @@ func extractRootBlockList(l ipc.RootBlockId_List) ([]root, error) {
return ids, nil
}

func (m DeleteResourcePush) handle(app *app) {
idsM, err := DeleteResourcePushT(m).Ids()
func (m RemoveResourcePush) handle(app *app) {
Copy link
Member

Choose a reason for hiding this comment

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

Is the delete to remove name change just for the sake of better names or there is a more explicit reason behind int?

Copy link
Member Author

Choose a reason for hiding this comment

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

it looks like a name change for the sake of a better name; i haven't found a collision with the previous name

@svv232 svv232 added the bitswap label Oct 1, 2024
@svv232
Copy link
Member Author

svv232 commented Oct 3, 2024

!ci-build-me

1 similar comment
@svv232
Copy link
Member Author

svv232 commented Oct 8, 2024

!ci-build-me

@svv232
Copy link
Member Author

svv232 commented Oct 8, 2024

!ci-nightly-me

@@ -332,7 +332,7 @@ struct Libp2pHelperInterface {
result @1 :ValidationResult;
}

struct DeleteResource {
struct RemoveResource {
Copy link
Member

Choose a reason for hiding this comment

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

UPD tag is part of encoded data, and root block id is hash of something derived from that data.

I.e. tag is sometimes provided to API for purposes of differentiating which function to use for the data. But for removal it isn't needed, data is perfectly referenced by the root block id alone.

@georgeee georgeee changed the title refactor delete to remove in libp2p bitswap and extend api of libp2p ipc for bitswap Bitswapo: rename deleteXX to removeXX, extend api of libp2p ipc Oct 14, 2024
@georgeee georgeee changed the title Bitswapo: rename deleteXX to removeXX, extend api of libp2p ipc Bitswap: rename deleteXX to removeXX, extend api of libp2p ipc Oct 14, 2024
Fix sending resource update to provide tag accourding to the new
Capnproto interface.
@georgeee georgeee changed the title Bitswap: rename deleteXX to removeXX, extend api of libp2p ipc Bitswap: rename deleteXX to removeXX, add tag to IPC Oct 14, 2024
Copy link
Member

@georgeee georgeee left a comment

Choose a reason for hiding this comment

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

Extended scope of PR: addition of tag implied that mkResourceUpdatedUpcall became incorrectly implemented.

I believe we need to bundle two updates together

Explain the implementation and extract some code out of the function for
better readability.
@georgeee
Copy link
Member

!ci-build-me

@svv232
Copy link
Member Author

svv232 commented Oct 16, 2024

!ci-build-me

@svv232 svv232 merged commit 3fed469 into compatible Oct 16, 2024
60 checks passed
@svv232 svv232 deleted the sai/bitswaplibp2p branch October 16, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants