Make Chain a variant of AccountOwner by deuszx · Pull Request #3560 · linera-io/linera-protocol

deuszx · 2025-03-13T10:01:37Z

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

Inline a Chain into AccountOwner enum to identify cases where transactions (mostly token transfers) are targeting or using chain's account balance. Previously that case was handled with the usage of Option<AccountOwner>. This made refactoring more difficult.

Test Plan

CI should catch regressions.

Release Plan

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

Links

reviewer checklist

linera-execution/src/lib.rs

bart-linera

I'm not sure I agree with the premise. It seems like a chain as a transfer target doesn't really belong as a variant of AccountOwner. If we want to be more explicit, I'd just make a type like enum TransferTarget { AccountOwner(AccountOwner), Chain } and replace Option<AccountOwner> with it. Then AccountOwner would play the role of identifying someone owning tokens that can authenticate transfers, and TransferTarget would represent, well, a target of a transfer.

I'm just not sure if that helps with solving the problem this was intended to solve, but I guess new potential variants of "addresses" could then be new variants of TransferTarget?

bart-linera · 2025-03-14T15:42:45Z

examples/amm/src/contract.rs

+            AccountOwner::Chain => {
+                unreachable!("Chains cannot be used to authenticate")
+            }


Those unreachable!s suggest to me that we might be conflating two different things here: who can authenticate a transaction, and where tokens can be transferred to. Maybe those should be represented by separate types? (Not necessarily something for this PR.)

AccountOwner::Chain is both a destination (the chain balance) and an ownership model (any block producer + temporarily message fees).

Here the issue is that unreachable! is inaccurate and the error message is kinda misleading. It should be something like

panic!("Using chain balance is not authorized")

Done in Replace unreachables with panics.

deuszx · 2025-03-14T15:57:09Z

I'm not sure I agree with the premise. It seems like a chain as a transfer target doesn't really belong as a variant of AccountOwner. If we want to be more explicit, I'd just make a type like enum TransferTarget { AccountOwner(AccountOwner), Chain } and replace Option<AccountOwner> with it. Then AccountOwner would play the role of identifying someone owning tokens that can authenticate transfers, and TransferTarget would represent, well, a target of a transfer.

I'm just not sure if that helps with solving the problem this was intended to solve, but I guess new potential variants of "addresses" could then be new variants of TransferTarget?

I think the ultimate solution would be to simply remove that variant entirely and either hardcode that every chain has an account with some hardcoded address ([0u8; 32]) where we store its balance or remove it - i.e. chains cannot have token balance at all.

bart-linera

After some discussion with Mateusz, it seems that a kind of separation between authenticating parties and owners already exists, and what drew my attention seems to have been implementation details of some example applications - so this change seems to be okay to me now 👍

bart-linera · 2025-03-14T16:27:52Z

linera-execution/src/lib.rs

-        application_id: UserApplicationId<A>,
+        application_id: ApplicationId<A>,


This (and below) seems to be a part of a different change?

bart-linera · 2025-03-14T16:31:44Z

linera-execution/src/system.rs

                authenticated_application_id == Some(owner),
                ExecutionError::UnauthenticatedClaimOwner
            ),
+            AccountOwner::Chain => unreachable!(),


Maybe it would make sense to handle this case? If a chain owner can transfer from an AccountOwner::Chain, then maybe it makes sense to let them claim from an AccountOwner::Chain, too? (if that's even possible to be implemented)

Sounds reasonable to mirror logic of transfer here. I didn't decide to do it b/c I would change the logic - i.e. the source here wasn't wrapped with Option.

Let's not do it in this PR, but I agree it sounds reasonable.

examples/fungible/src/contract.rs

examples/native-fungible/src/contract.rs

ma2bd · 2025-03-14T21:52:08Z

examples/native-fungible/src/contract.rs

@@ -144,6 +144,9 @@ impl NativeFungibleTokenContract {
                );
            }
            AccountOwner::Application(_) => panic!("Applications not supported yet"),


We should fix that (not here but soon)

linera-base/src/identifiers.rs

examples/non-fungible/src/contract.rs

ma2bd · 2025-03-14T21:54:29Z

linera-base/src/identifiers.rs

        match self {
            AccountOwner::User(owner) => write!(f, "User:{}", owner)?,
            AccountOwner::Application(app_id) => write!(f, "Application:{}", app_id)?,
+            AccountOwner::Chain => write!(f, "Chain")?,


I look forward to removing these tags

The ::Chain variant might not be so easy to remove b/c there are flow paths where the Chain is still a valid identifier. Right now I can see two ways to get rid of it:

Do not let chain own a balance.

Turn ::Chain variant into a special case of address ([u8; 32]).

The first one isn't just a refactor (internal change) as it requires making some high-level design decisions around opening new chains etc.

I think the second option is the easier one and we should try to do it. We could hash a string like "Linera Chain Address" (note it cannot simply be [0u8; 32] b/c in Ed25519 a private key for all zeros public key is well-known).

note it cannot simply be [0u8; 32] b/c in Ed25519 a private key for all zeros public key is well-known)

Addresses are not public keys but hash values, so [0u8; 32] would work. I proposed 0x0 (Reserved(0)) in the internal doc.

linera-core/src/chain_worker/state/temporary_changes.rs

ma2bd

Thanks for the PR. Very nice improvement. No major issue except that refund_grand_to should be an option.

ma2bd · 2025-03-14T22:02:51Z

linera-execution/src/system.rs

                        context.authenticated_signer,
                        None,
-                        owner.map(AccountOwner::User),
+                        owner.map(AccountOwner::User).unwrap_or(AccountOwner::Chain),


We should instead modify SystemOperation to use SystemOperation::Transfer { owner: Owner .. } and resolve the default value way earlier.

unwrap_or(AccountOwner::Chain) is a little bit of a red flag. After this PR, I would imagine that only CLI options may default to a chain account.

I agree it looks dangerous. So I tracked the places where I use AccountOwner::Chain as the default - there are two (actually three but there are two instances where I use ::Chain as source when constructing a SystemMessage::Credit so that's one for the purpose of this discussion):

In send_refund we previously were using None as source and now we use AccountOwner::Chain.

In system.rs#execute_operation we previously were using None as source for the transfer and now we default to ::Chain.

In both those cases None resolved to the "chain" already:

main vs this PR

main vs this PR.

There's no regression here.

Also, see #3573 where I replace Transfer::source: Option<Owner> with AccountOwner and get rid of some of these unwrap_or(AccountOwner::Chain).

Thanks for checking. Indeed the source of a refund's Credit message is currently a best effort. (It only matters if the refund message itself is rejected.)

linera-execution/src/lib.rs

linera-execution/src/runtime.rs

ma2bd · 2025-03-14T22:04:42Z

linera-execution/src/system.rs

        let mut outcome = RawExecutionOutcome {
            authenticated_signer: context.authenticated_signer,
-            refund_grant_to: context.refund_grant_to(),
+            refund_grant_to: Some(context.refund_grant_to()),


No. We should not force refunds.

Ah, you're right. We actually had two Options previously here - the outer one (return type) and inner one (Some(Account { owner: None })) and previously Some(None) would not refund at all.

Do not force refunds to chain

ma2bd · 2025-03-15T23:19:17Z

linera-core/src/chain_worker/state/temporary_changes.rs

+                    chain.execution_state.system.balances.get(&owner).await?;
+            }
+            AccountOwner::Chain => {
+                info.requested_owner_balance = Some(*chain.execution_state.system.balance.get());


Here or very soon we should remove SystemExecutionStateView::balance by the way

I took a quick stab at removing the chains' balances entirely and it wasn't very difficult. I can propose something in a separate PR.

ma2bd · 2025-03-15T23:37:33Z

linera-execution/src/system.rs

                        context.authenticated_signer,
                        None,
-                        owner.map(AccountOwner::User),
+                        owner.map(AccountOwner::User).unwrap_or(AccountOwner::Chain),


Thanks for checking. Indeed the source of a refund's Credit message is currently a best effort. (It only matters if the refund message itself is rejected.)

deuszx force-pushed the cleanup-account-owner-chain branch 4 times, most recently from 795c626 to d66cb15 Compare March 13, 2025 12:55

deuszx added 3 commits March 13, 2025 12:57

Make Chain a variant of AccountOwner

7e2fb52

Use UserApplicationId type where appropriate

bc2626b

Return Account from grant_refund_to

85f269f

deuszx force-pushed the cleanup-account-owner-chain branch from d66cb15 to 85f269f Compare March 13, 2025 12:57

deuszx commented Mar 13, 2025

View reviewed changes

linera-execution/src/lib.rs Outdated Show resolved Hide resolved

deuszx requested review from afck and bart-linera March 14, 2025 14:40

deuszx marked this pull request as ready for review March 14, 2025 14:40

bart-linera reviewed Mar 14, 2025

View reviewed changes

bart-linera approved these changes Mar 14, 2025

View reviewed changes

linera-io deleted a comment from deuszx Mar 14, 2025