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

Remove FederationInfo #4297

Merged
merged 1 commit into from
Feb 23, 2024
Merged

Remove FederationInfo #4297

merged 1 commit into from
Feb 23, 2024

Conversation

joschisan
Copy link
Contributor

Many methods were never used, the invite_code field was never read. Every time this struct was created from an invite code the caller just wanted to obtain the client config.

@joschisan joschisan requested review from a team as code owners February 11, 2024 10:35
Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

The whole point of this struct is to allow 3rd party integrators to gather some basic information about the federation to show to users before joining. I guess we could try to merge its functionality with the ClientConfig type now that we can generate InviteCodes from config again (that wasn't possible when this type was created).

@joschisan
Copy link
Contributor Author

I think porting the functionality to the client config makes sense as FederationInfo is practically only a wrapper around the config since we can generate invite codes from the client config. What functionality would you like to see ported?

@elsirion
Copy link
Contributor

The whole public interface except from_config I guess, since there was a reason to add ever fn (not inside our code base, but really useful when building a client app).

@joschisan joschisan force-pushed the rm_fed_info branch 2 times, most recently from b51d334 to a7a18f4 Compare February 12, 2024 18:49
@joschisan joschisan force-pushed the rm_fed_info branch 4 times, most recently from 5012799 to 849fa63 Compare February 15, 2024 06:43
@@ -2214,29 +2159,66 @@ pub async fn get_invite_code_from_db(db: &Database) -> Option<InviteCode> {

/// Tries to download the client config from the federation,
/// attempts up to `retries` number times
Copy link
Contributor

Choose a reason for hiding this comment

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

retries isn't an argument anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

invite_code: InviteCode,
max_retries: usize,
) -> anyhow::Result<ClientConfig> {
pub async fn download_client_config(invite_code: &InviteCode) -> anyhow::Result<ClientConfig> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this a constructor/member of ClientConfig to replicate the FederationInfo interface. I.e. the user should be able to call ClientConfig::download_with_invite(invite) or something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

DynGlobalApi::from_config(self)
}

pub fn invite_code(&self, peer: &PeerId) -> Option<InviteCode> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation needed: what does the peer argument do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@elsirion elsirion dismissed their stale review February 15, 2024 18:20

Makes sense, just not perfect yet

elsirion
elsirion previously approved these changes Feb 19, 2024
}

/// Tries to download the client config from the federation,
/// attempts to retry teb times before giving up.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: s/teb/ten/?

Copy link
Contributor

Choose a reason for hiding this comment

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

still relevant, but whatever …

@elsirion
Copy link
Contributor

Needs rebase, feel free to request review from others in the future when I'm out :)

}

/// Tries to download the client config from the federation,
/// attempts to retry teb times before giving up.
Copy link
Contributor

Choose a reason for hiding this comment

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

still relevant, but whatever …

@elsirion elsirion requested a review from dpc February 23, 2024 11:56
@elsirion
Copy link
Contributor

Might be worth incorporating #4371 into this PR too or a follow-up.

@justinmoon justinmoon added this pull request to the merge queue Feb 23, 2024
Merged via the queue into fedimint:master with commit 51e28b2 Feb 23, 2024
20 checks passed
@joschisan joschisan deleted the rm_fed_info branch February 23, 2024 14:20
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.

None yet

3 participants