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

Separate types for Application and ApplicationId #3570

Closed
wants to merge 2 commits into from

Conversation

deuszx
Copy link
Contributor

@deuszx deuszx commented Mar 13, 2025

Motivation

We want to fix and expand our notion of Address (currently Owner, GenericApplicationId, etc.) to different type of addresses (32-byte Linera/Solana, 20-byte EVM). In order to do that we need to prepare the code for the introduction of new variants.

Proposal

Add a separate type for ApplicationId(hash) vs Application (a hash + type-safe ABI). Previously the presence of A type parameter in "address" enum made it more difficult to refactor. After this PR we have separate types for application's address and application with ABI (used mostly in tests and CLI now).

Test Plan

CI should catch regressions.

Release Plan

  • Nothing to do / These changes follow the usual release cycle.

Links

@deuszx deuszx force-pushed the separate-application-id branch 4 times, most recently from b161afc to 2bf2eb2 Compare March 13, 2025 20:51
@ma2bd
Copy link
Contributor

ma2bd commented Mar 14, 2025

We haven't really discussed what to do with ApplicationId yet. I was hoping we don't need to change this.

@deuszx
Copy link
Contributor Author

deuszx commented Mar 14, 2025

We haven't really discussed what to do with ApplicationId yet. I was hoping we don't need to change this.

We can discuss but I don't see what's controversial here. We keep all the security and features as before - users can use type-safe Application - it's just now the address of the application doesn't have the type parameter. In my mind that's not a regression since we were calling forget_abi() anyway (i.e. we set A = () for UserApplicationId).

@deuszx deuszx force-pushed the separate-application-id branch 3 times, most recently from dbb70cd to b7c5975 Compare March 14, 2025 14:39
@deuszx deuszx changed the base branch from cleanup-account-owner-chain to transfer-use-accountowner-type March 14, 2025 14:40
@deuszx deuszx requested review from afck, jvff and ma2bd March 14, 2025 14:41
@deuszx deuszx marked this pull request as ready for review March 14, 2025 14:41
@deuszx deuszx force-pushed the transfer-use-accountowner-type branch from eef4aff to 9aee4a1 Compare March 14, 2025 14:51
@deuszx deuszx force-pushed the separate-application-id branch from b7c5975 to 734f9ec Compare March 14, 2025 14:51
Copy link
Contributor

@ma2bd ma2bd left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

I'd rather keep application_id: ApplicationId<A> for the SDK: Application feels a bit vague and requires a massive refactoring of variable names -- including for our SDK users.

Why not use UserApplicationId for the (internal) use case without a type parameter?

@@ -113,7 +113,7 @@ impl EthereumTrackerContract {
r#"query {{ readTransferEvents(endBlock: {end_block}) }}"#
));

let application_id = self.runtime.application_id();
let application_id = self.runtime.application();
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR ruins the field/variable naming convention in many places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #3582 for alternative.

@deuszx
Copy link
Contributor Author

deuszx commented Mar 14, 2025

I'd rather keep application_id: ApplicationId<A> for the SDK: Application feels a bit vague and requires a massive refactoring of variable names -- including for our SDK users.

In my mind *Id is just for identification and so the ApplicationId<A: Abi> whose purpose (and difference compared to ApplicationId<()>) was to convey the underlying application's ABI did not seem fit that as the additional Abi type provided information about the application that goes beyond its address.

Why not use UserApplicationId for the (internal) use case without a type parameter?

What does the User* prefix stand for? An application without the typed ABI (ApplicationId<()>) becomes a User* one when we erase it (the ABI). The same application (the same underlying address) is either an application (when the ABI is typed - ApplicationId<A>) and user application (when it's not typed - ApplicationId<()>). I can use UserApplicationId for the untyped struct but this comment would be valid still.

@deuszx deuszx force-pushed the transfer-use-accountowner-type branch from 9aee4a1 to 3ab8c5a Compare March 16, 2025 16:41
Base automatically changed from transfer-use-accountowner-type to main March 16, 2025 18:41
@deuszx
Copy link
Contributor Author

deuszx commented Mar 17, 2025

Closing this in favor of #3582 which has far fewer changes (avoids client-facing renames).

@deuszx deuszx closed this Mar 17, 2025
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.

2 participants