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

Adds addresses field to gettowerinfo #151

Merged
merged 2 commits into from
Nov 25, 2022

Conversation

sr-gi
Copy link
Member

@sr-gi sr-gi commented Nov 12, 2022

This PR adds a new field to the gettowerinfo RPC call. The field includes data regarding the interfaces where the public API is being offered (i.e. ipv4 and/or tor).

The rationale for this is having the API endpoint information handy without having to check the logs (this is currently most relevant for the onion address).

Fixes #143

cc/ @jochemin

@sr-gi
Copy link
Member Author

sr-gi commented Nov 12, 2022

Notice this is a breaking change for the protos, given it modifies them without updating its version. I don't think it is worth a version bump for them at the moment, maybe when more functionality has been added (or after the next major update).

@sr-gi sr-gi requested a review from mariocynicys November 12, 2022 18:48
@sr-gi
Copy link
Member Author

sr-gi commented Nov 16, 2022

Something I'd like to discuss here would be how to address proto updates without them being breaking changes. I haven't really paid much attention to this so far, and maybe we don't want to, but there are ways of making it backward compatible.

https://developers.google.com/protocol-buffers/docs/proto3#updating

@sr-gi sr-gi force-pushed the gettowerinfo_addresses branch from a3131f8 to 644be32 Compare November 24, 2022 16:02
@sr-gi
Copy link
Member Author

sr-gi commented Nov 24, 2022

Notice this is a breaking change for the protos, given it modifies them without updating its version. I don't think it is worth a version bump for them at the moment, maybe when more functionality has been added (or after the next major update).

Something I'd like to discuss here would be how to address proto updates without them being breaking changes. I haven't really paid much attention to this so far, and maybe we don't want to, but there are ways of making it backward compatible.

https://developers.google.com/protocol-buffers/docs/proto3#updating

I've rebased this and made the change non-breaking. This was breaking due to the field order, and the only good reason for the ordering was having addresses close to tower_id when serializing in json to show the result to the user. I don't think that actually reasonable, if we want a different ordering for the displayed result we can always implement our own serialization.

I've also moved the AddressType logic to common so it can be reused by the clients (e.g. to flag a given address as clearnet or tor). This is relevant to address the Tor related issues from #102 in a more elegant way.

TL;DR: this is non-breaking now. AddressType is now part of teos-common.

Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

LGTM :)

teos/build.rs Outdated Show resolved Hide resolved
teos/build.rs Show resolved Hide resolved
Comment on lines 142 to 147
loop {
sleep(Duration::from_secs(1)).await;
if shutdown_signal_tor.is_triggered() {
break;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be replace with

shutdown_signal_tor.await;

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, nice one! I never liked that chuck of code

@sr-gi sr-gi force-pushed the gettowerinfo_addresses branch from 644be32 to 9ae985e Compare November 25, 2022 12:14
@sr-gi
Copy link
Member Author

sr-gi commented Nov 25, 2022

Should be good to go now

@sr-gi sr-gi force-pushed the gettowerinfo_addresses branch 2 times, most recently from d23a5f9 to 8477075 Compare November 25, 2022 12:52
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

LGTM otherwise!

Comment on lines 139 to 147

// // NOTE: Needed to keep connection with control port & hidden service running, as soon as we leave
// // this function the control port stream is dropped and the hidden service is killed
// loop {
// sleep(Duration::from_secs(1)).await;
// if shutdown_signal_tor.is_triggered() {
// break;
// }
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still here 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops

@sr-gi sr-gi force-pushed the gettowerinfo_addresses branch from 8477075 to 0c1b4c1 Compare November 25, 2022 13:27
@sr-gi sr-gi merged commit c14c736 into talaia-labs:master Nov 25, 2022
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.

Add onion address on get_tower_info
2 participants