Simplified account details for on-chain accounts (only delta remains) #530

polydez · 2024-03-20T08:01:12Z

Related to issue: #524

We decided to use just AccountDelta for on-chain accounts, even for new ones. In this case, all values must be provided.

Limitations:

Code is supported only for new accounts. Code updates are still not supported.
Storage for new on-chain accounts may have only simple layout (set of values created from updated_values). Maps and arrays are not supported.

I think, we also should discuss these limitations. Do we really want to move in that direction? It seems, we would need to add more and more special cases for delta processing for new/existing accounts so this will lead to even worse code than it was before simplification.

bobbinth · 2024-03-20T22:19:32Z

As I mentioned in #524, I think we should probably put this on hold for now.

But as far as this PR goes, we'd also need to update deserialization code for AccountStorageDelta as it fail to deserialize "immutable" slot updates.

bobbinth

Thank you! Looks good. Not a full review yet - but I left some comments inline.

bobbinth · 2024-04-18T10:31:54Z

CHANGELOG.md

@@ -2,6 +2,8 @@

 ## 0.3.0 (TBD)

+* [BREAKING] Simplified account details for on-chain accounts, now only delta provided


minor nit: let's add a PR number to this line (similar to how we do it for other changes below).

bobbinth · 2024-04-18T10:34:00Z

objects/src/accounts/delta/mod.rs

 pub struct AccountDelta {
    storage: AccountStorageDelta,
    vault: AccountVaultDelta,
+    code: Option<AccountCode>,
    nonce: Option<Felt>,
 }


We should also update comments for this struct.

bobbinth · 2024-04-18T10:37:22Z

objects/src/accounts/delta/storage.rs

@@ -31,7 +31,11 @@ impl AccountStorageDelta {
    /// - The number of cleared or updated items is greater than 255.
    /// - Any of cleared or updated items are at slot 255 (i.e., immutable slot).
    /// - Any of the cleared or updated items is referenced more than once (e.g., updated twice).
-    pub fn validate(&self) -> Result<(), AccountDeltaError> {
+    pub fn validate(&self, is_new_account: bool) -> Result<(), AccountDeltaError> {


Let's update the comments above to include this new error condition.

bobbinth · 2024-04-18T10:41:20Z

objects/src/accounts/delta/mod.rs

+        if nonce != Some(ZERO) {
+            return Err(AccountDeltaError::IncorrectNonceForNewAccount(nonce));
+        }


I'm not sure this is correct: nonce for a new account would start out as zero, but after the update it would not be zero. So, the nonce in the delta would not be zero. We could potentially force the nonce for the first update to be one - but I'm not sure that's a good idea.

bobbinth · 2024-04-18T10:43:32Z

objects/src/accounts/delta/storage.rs

Let's make sure deserialization code here works correctly. I think this will currently fail for new accounts as for new accounts we need to set storage layout in MAX_MUTABLE_STORAGE_SLOT_IDX but deserialization code would not allow this.

bobbinth · 2024-04-18T10:44:19Z

objects/src/accounts/delta/vault.rs

@@ -28,7 +28,11 @@ impl AccountVaultDelta {
    /// - The number of removed assets is greater than [u16::MAX].
    /// - The same asset was added more than once, removed more than once, or both added and
    ///   removed.
-    pub fn validate(&self) -> Result<(), AccountDeltaError> {
+    pub fn validate(&self, is_new_account: bool) -> Result<(), AccountDeltaError> {


Let's update the comments above describing the new failure condition.

bobbinth · 2024-04-18T10:46:54Z

objects/src/accounts/mod.rs

+    /// Creates and returns a new account constructed from the specified ID and delta.
+    pub fn from_delta(id: AccountId, delta: &AccountDelta) -> Result<Self, AccountError> {


We should probably return an error from here if the delta is not for new account.

bobbinth · 2024-04-18T10:51:24Z

objects/src/accounts/mod.rs

+        let storage = AccountStorage::new(
+            delta
+                .storage()
+                .updated_items
+                .iter()
+                .map(|&(index, value)| SlotItem {
+                    index,
+                    slot: StorageSlot::new_value(value),
+                })
+                .collect(),
+        )?;


This may be fine for now - but we'll also need to handle other storage slot types (e.g., storage map which are implemented in #521). So, I wonder if it would be better to encapsulate this in the AccountStorageDelta - e.g., by implementing TryFrom<AccountStorageDelta> for <AccountStorage>.

polydez · 2024-04-18T12:14:37Z

@bobbinth thank you!
@bobbinth @hackaugusto, what do you think, are we moving the right direction with substituting account/delta with just delta? It seems, that delta doesn't fit well for new accounts. It's possible that eventually we will need to rollback to our previous approach.

# Conflicts: # CHANGELOG.md # objects/src/transaction/proven_tx.rs

bobbinth · 2024-04-19T07:17:51Z

@bobbinth @hackaugusto, how do you think, are we moving the right direction with substituting account/delta with just delta? It seems, that delta doesn't fit well for new accounts. It's possible that eventually we will need to rollback to our previous approach.

Yeah - I was wondering the same thing. It seems like we are introducing a lot of code to handle the special case of AccountDelta being for a new account, and maybe having new vs. existing cases separated the way we have now is cleaner approach.

If we do keep the things the way they are now, we should probably rename AccountDetails into AccountUpdateDetails, and maybe define it as:

pub enum AccountUpdateDetails {
    None,
    NewAccount(Account),
    Delta(AccountDelta),
}

Another potential thing we might want to do is to define a new struct for AccountUpdate - something like:

pub struct AccountUpdate {
    init_hash: Diget,
    final_hash: Digest,
    details: AccountUpdateDetails,
}

And then ProvenTransaction could look like:

pub struct ProvenTransaction {
    id: TransactionId,
    account_id: AccountId,
    account_update: AccountUpdate,
    input_notes: InputNotes<Nullifier>,
    output_notes: OutputNotes,
    block_ref: Digest,
    proof: ExecutionProof,
}

polydez · 2024-04-19T12:14:30Z

@bobbinth thank you!
@hackaugusto what do you think?

hackaugusto · 2024-04-19T14:12:35Z

@hackaugusto what do you think?

I don't really understand why the special cases are needed. But if you and @bobbinth think the change is worse, totally fine with me to not do it.

bobbinth · 2024-04-22T08:12:56Z

I don't really understand why the special cases are needed.

There are a couple of reasons. First is validation of the delta. For example, for new accounts, the vault delta should not have any removed_assets. Second reason is about how we instantiate the account. For example, we need to have a special constructor (e.g., Account::from_delta(account_id, account_delta)) since we can't just create a blank account and apply delta to it.

If we forego special cases during validation, that would probably simplify things enough to make it worth it. But I'm not sure if that's a good idea.

polydez · 2024-04-22T14:54:02Z

Closed in favour of #618

polydez requested review from hackaugusto and bobbinth March 20, 2024 08:01

polydez marked this pull request as ready for review March 20, 2024 12:08

polydez self-assigned this Mar 20, 2024

bobbinth mentioned this pull request Mar 20, 2024

Simplify AccountDetails in ProvenTransaction #524

Closed

feat: simplify account details, only delta is needed

dc072b2

polydez force-pushed the polydez-simplify-account-details branch from beea8af to dc072b2 Compare April 16, 2024 16:29

polydez added 2 commits April 16, 2024 21:30

feat: implement account delta validation for new accounts

e7b673a

feat: improve validation

450e317

polydez requested a review from igamigo April 18, 2024 05:09

bobbinth reviewed Apr 18, 2024

View reviewed changes

Merge branch 'next' into polydez-simplify-account-details

d44e8ec

# Conflicts: # CHANGELOG.md # objects/src/transaction/proven_tx.rs

polydez mentioned this pull request Apr 22, 2024

Refactored account update details #618

Closed

polydez closed this Apr 22, 2024

hackaugusto deleted the polydez-simplify-account-details branch April 22, 2024 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplified account details for on-chain accounts (only delta remains) #530

Simplified account details for on-chain accounts (only delta remains) #530

polydez commented Mar 20, 2024 •

edited

Loading

bobbinth commented Mar 20, 2024

bobbinth left a comment

bobbinth Apr 18, 2024

bobbinth Apr 18, 2024

bobbinth Apr 18, 2024

bobbinth Apr 18, 2024

bobbinth Apr 18, 2024

bobbinth Apr 18, 2024

bobbinth Apr 18, 2024

bobbinth Apr 18, 2024

polydez commented Apr 18, 2024 •

edited

Loading

bobbinth commented Apr 19, 2024

polydez commented Apr 19, 2024

hackaugusto commented Apr 19, 2024

bobbinth commented Apr 22, 2024

polydez commented Apr 22, 2024

		@@ -2,6 +2,8 @@

		## 0.3.0 (TBD)

		* [BREAKING] Simplified account details for on-chain accounts, now only delta provided

		/// Creates and returns a new account constructed from the specified ID and delta.
		pub fn from_delta(id: AccountId, delta: &AccountDelta) -> Result<Self, AccountError> {

Simplified account details for on-chain accounts (only delta remains) #530

Simplified account details for on-chain accounts (only delta remains) #530

Conversation

polydez commented Mar 20, 2024 • edited Loading

bobbinth commented Mar 20, 2024

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polydez commented Apr 18, 2024 • edited Loading

bobbinth commented Apr 19, 2024

polydez commented Apr 19, 2024

hackaugusto commented Apr 19, 2024

bobbinth commented Apr 22, 2024

polydez commented Apr 22, 2024

polydez commented Mar 20, 2024 •

edited

Loading

polydez commented Apr 18, 2024 •

edited

Loading