Account storage not updated on using `set_item` api in target account context

nlok5923 commented 2 months ago

What should be done?

I am using a custom notescript to store some information in a regular immutable account at particular slot index using set_item api.

Note script Code:

use.miden::account
use.miden::note
use.miden::contracts::wallets::basic->wallet
use.miden::contracts::poker::poker

proc.receive_cards
    exec.account::set_item

    # prepare stack for return
    movup.4 drop movup.4 drop movup.4 drop
end

begin
    # All scripts start with the NOTE_SCRIPT_ROOT, you can drop it
    dropw 

    push.0 exec.note::get_inputs 
    # debug.stack
    # it saves first word at zero and the continues the second word at 1 
    # => [num_inputs, des_pointer, ...]

    # dup push.8 eq assert (just for debugging purposes)
    # => [des_pointer = 0, ...]

    push.0
    # since now the word is at position 0 and 1 for loading the first word we will push zero and then load the word      
    mem_loadw 
    push.100 
    debug.stack
    # truncate_stack
    call.receive_cards # using call we are storing the cards in memory of the account context

    push.1
    # since now the word is at position 0 and 1 for loading the first word we will push one and then load the word 
    mem_loadw push.101 

    call.receive_cards

    # Debugging https://0xpolygonmiden.github.io/miden-vm/user_docs/assembly/debugging.html
    debug.stack # log the entire stack content

    # All account and note procedures
    # https://0xpolygonmiden.github.io/miden-base/transactions/transaction-procedures.html#transaction-procedures 

    push.0 exec.note::get_assets drop mem_loadw
    # => [ASSET, ...]

    # load the asset and add it to the account
    call.wallet::receive_asset
    # => [...]

    dropw
end

Account Code where we want to store the card information

use.miden::contracts::wallets::basic->basic_wallet
use.miden::contracts::auth::basic->basic_eoa
use.miden::account

export.basic_wallet::receive_asset
export.basic_wallet::send_asset
export.basic_eoa::auth_tx_rpo_falcon512
# export.account::set_item

export.receive_cards

    exec.account::set_item

    # prepare stack for return
    movup.4 drop movup.4 drop movup.4 drop
end

I am calling receive_cards procedure in notescripts to set custom information (cards informtion which is a word) in player account storage in slot 100 and 101.

How should it be done?

Even after setting the account storage using set_item api the storage root of the account doesn't change

Before consumption acc storage root RpoDigest([11169760792063196321, 7115779431449406272, 3275039973163998971, 7283060719841967182])
After consumption acc storage root RpoDigest([11169760792063196321, 7115779431449406272, 3275039973163998971, 7283060719841967182])

When is this task done?

The storage root of account should change and we should be able to see card information on slot 100 and 101 of account storage.

Additional context

Steps to reproduce:

Clone the repo: https://github.com/Rizelabs/aze-server
Start local miden node
cd aze-server
change branch git checkout feat/game-txn-call
Run the server cargo run --release
Hit endpoint curl -X GET http://localhost:8000/v1/game/create-account

Notescript: https://github.com/RizeLabs/aze-server/blob/feat/game-txn-call/lib/contracts/notes/game/deal.masm

bobbinth commented 2 months ago

@nlok5923 - thank you for filing such a detailed report!

@Dominik1999 - could you check this? I thought we had some tests in miden-base that tested account:set_item procedure - but maybe there are some things we are not testing there.

mFragaBA commented 2 months ago

Hi! Thanks for the detailed report.

I added a few more debug statements on the note script and the code:

during note script execution it seems that when set_item is called the proper stack arguments are there
after tx execution, I checked the account delta and the updated items on the account's store are set:

updated_items: [
            (
                100,
                [
                    1,
                    1,
                    0,
                    0,
                ],
            ),
            (
                101,
                [
                    1,
                    2,
                    0,
                    0,
                ],
            ),
        ],

I think the issue is that after executing the transactions and syncing, you need to re-load the account from the store. Otherwise the updates won't go through:

    // check the store of player 1 account to see are the cards set properly

   // ======== I ONLY ADDED THESE 2 LINES ==========
    let (player_account, _) = client.get_account(player_account.id()).unwrap();
    let player_account_storage = player_account.storage();

    // for i in 0..102 {
    //     println!("Player account storage {:?} {:?}", i , player_account_storage.get_item(i));
    // }
    let after_acc_storage = player_account.storage().root();

    println!("Player Card 1 {:?}", player_account_storage.get_item(100));
    println!("Player Card 2 {:?}", player_account_storage.get_item(101));
    println!("Prev acc storage root {:?}", prev_acc_storage);
    println!("After txn acc storage root {:?}", after_acc_storage);

And I got the proper output (both the cards are correct and the storage root hash seems to have changed):

Player Card 1 RpoDigest([1, 1, 0, 0])
Player Card 2 RpoDigest([1, 2, 0, 0])
Prev acc storage root RpoDigest([691325485839961784, 18292302958153520671, 5992886236460359113, 743126542260494113])
After txn acc storage root RpoDigest([7246666647329329192, 14337335527969961969, 12753666777987838473, 5246371044914077319])

Let me know it it solves it for you @nlok5923 otherwise we'll figure it out

nlok5923 commented 2 months ago

Thanks @mFragaBA for the help!

I think the issue is fixed now, Shouldn't we make Account api's such that it should always fetch the latest values from the client store ?

mFragaBA commented 2 months ago

Thanks @mFragaBA for the help!

I think the issue is fixed now, Shouldn't we make Account api's such that it should always fetch the latest values from the client store ?

On the one hand, it would ease the developer using miden-client because they wouldn't have to worry on keeping the most up to date version of the account.

But on the other hand, Account comes from miden-base and is used in other places which are not the client. We could make a wrapper type for account, but that would mean the account should have some sort of reference to either the store or the client. That would also in turn add more complexity. Plus you'd be forcing a request onto a db each time you want to access an element.

In any case it is possible for developers to write a wrapper type, something around this:

pub struct AccountWrapper<'s, N: NodeRpcClient, R: FeltRng, S: Store> {
    client: &'s Client<N, R, S>,
    account: Account,
}

impl<'s, N: NodeRpcClient, R: FeltRng, S: Store> AccountWrapper<'s, N, R, S> {
    pub fn code(&self) -> &AccountCode {
        todo!()
    }

    pub fn vault(&self) -> &AssetVault {
        todo!()
    }

    pub fn storage(&mut self) -> &AccountStorage {
        let (account, _seed) = self.client.get_account(self.account.id()).unwrap();
        self.account = account;
        self.account.storage()
    }
}

mFragaBA commented 2 months ago

you can also avoid going to the db. If you have an Account and the TransactionResult you can also do

// assuming you have mutable access to the account
account.apply_delta(transaction_result.account_delta()).unwrap(); // proper error handling missing

mFragaBA commented 2 months ago

closing the issue as the original problem got solved.

0xPolygonMiden / miden-client