Data Location Assignment [OLD]

Ethereum Smart Contract Programming 101
#345

This is excellent @nielvosloo! :star_struck:

It’s great how you’ve extended the assignment to effectively address the weaknesses and limitations in the original basic contract, so that now…
(i) users without an existing account can’t have balances updated;
(ii) existing users can’t have their account balance overwritten by a new user with the same id; and
(ii) users without an existing account won’t have a zero balance returned by the getter.

All of these issues are resolved skillfully and effectively by your solution, which also generates appropriate error messages depending on the situation, and all handled by just one modifier. You’ve also practised and shown that you can use and adapt modifiers, require() and assert()…

… oh, and your solution to the original problem is also correct, the most concise and also uses less gas than some of the alternatives :smiley:

Well done! Fantastic progress! :muscle:

2 Likes
#346

Thanks a million for the detailed feedback @jon_m, really appreciate it!

1 Like
#347

Relevant change:

function updateBalance(uint id, uint balance) public {
     users[id].balance = users[id].balance + balance;
}
1 Like
#348

pragma solidity 0.5.12;
contract MemoryAndStorage {

mapping(uint => User) users;

struct User{
    uint id;
    uint balance;
}

function addUser(uint id, uint balance) public {
    users[id] = User(id, balance);
}

function updateBalance(uint id, uint balance)  public {
     users[id].balance = balance;
}

function getBalance(uint id) view public returns (uint) {
    return users[id].balance;
}

}

1 Like
#349

Hi @SpiritOfThe9,

That works if you want to update the balance by adding the input balance to the existing balance. If that’s what you want to do then I would suggest changing the parameter name to something different to balance, as you aren’t adding the account balance, but a deposit amount e.g. uint deposit. This will make what your function is doing clearer I think. Also you can avoid repeating  users[id].balance   by using the += assignment operator as follows:

function updateBalance(uint id, uint deposit) public {
     users[id].balance += deposit;
}
1 Like
#350

Thx for feedback.
All what you are saying makes sense.

Of course, the explanation video makes it clear that the intent was not to add, only update, and thus:

function updateBalance(uint id, uint balance) public {
     users[id].balance = balance;
}

is a solution.

1 Like
#351

Hey, @SpiritOfThe9,

Yours was a very logical approach, though. After all, calculations involving balances more commonly add or substract amounts from them (i.e. deposits or withdrawals), rather than replace them with a new total. I didn’t mean you should change your solution to one of the standard ones, but rather suggesting how you can adapt your own, so that it is more suited to your alternative approach. It’s also good to see a variety of different approaches and solutions posted here in the forum discussion threads, as it makes it a more interesting resource.

Stay creative! :smiley:

1 Like
#352

pragma solidity 0.5.12;
contract MemoryAndStorage {

mapping(uint => User) users;
uint[] private userIds;

address owner;

struct User{
    uint id;
    uint balance;
}

modifier onlyOwner() {
    require(msg.sender == owner);
    _;
}

constructor() public {
    owner = msg.sender;
}

function addUser(uint id, uint balance) public onlyOwner {
    users[id] = User(id, balance);
    userIds.push(id);
}

function updateBalance(uint id, uint balance) public onlyOwner {
     require(users[id].id == id, "User ID does not exist");
     
     users[id].balance = balance;
     assert(users[id].balance == balance);
}

function getBalance(uint id) view public returns (uint) {
    return users[id].balance;
}

function getUserIds() view public onlyOwner returns (uint[] memory) {
    return userIds;
}

}

1 Like
#353

pragma solidity 0.5.1;
contract MemoryAndStorage {

mapping(uint => User) users;

struct User{
    uint id;
    uint balance;
}

function addUser(uint id, uint balance) public {
    users[id] = User(id, balance);
}

function updateBalance(uint id, uint balance)  public {
    users[id].balance = balance;
}

function getBalance(uint id) view public returns (uint) {
    return users[id].balance;
}

}

1 Like
#354 
 function updateBalance(uint id, uint balance) public {
        users[id].balance = balance;
    }
1 Like
#355

In this function I changed memory to storage

function updateBalance(uint id, uint balance) public {
User storage user = users[id]; //<------ Changed from memory to storage
user.balance = balance;
}

1 Like
#356

Hi @massam78,

Nice additional code, as an extension to the assignment :ok_hand:

You may want to also consider the following additions:

  • Mark the owner variable with either public or private visibility.

  • Add a require() function to the getBalance function (as well as the updateBalance function), so that a zero balance isn’t returned for a User account ID that does not exist.

1 Like
#357

function updateBalance(uint id, uint balance) public{

    user[id].balance = balance;
}
#358

Hi @cryptocoder,

This won’t work because you don’t have a mapping with the name user. The mapping is users. You may just want to correct that :wink:

1 Like
#359

thanks i was trying to find the problem.

i created a new file memorystorage.sol.
but unable to deploy the contract in version 0.6.6 .
can any one guide me

pragma solidity 0.6.6;
contract MemoryAnd Storage {

mapping(unit => User) users;

struct User{

    uint id;
    uint balance;
    
}

function addUser(uint id,uint balance) public {
    users[id] = User(id,balance);
}

function updateBalance(uint id, uint balance) public{
           
    user[id].balance = balance;
}

function getBalance(uint id) view public returns (uint) {
     return users[id].balance;
}

}

#360

i found the problems .
it was not named properly in new file .

1 Like
#361

Yes, @cryptocoder, I think you’ve got it now. Because your mapping has the name users and not user:

You need to change…

to

users[id].balance = balance;

By just adding the ‘s’ your contract will execute in version 0.6.6 :slightly_smiling_face:

1 Like
#362

I added:

    //Solution 1
     User memory user = users[id];
     user.balance = balance;
     users[id].balance= user.balance;
     
     //Solution 2 remove the Use memory user....
     users[id].balance = balance;
1 Like
#363

Hi @elterremoto,

Welcome to the forum! Good to see you here!

This works, but as we have already assigned balance to user in the 2nd line…

user.balance = balance;

… you only need to add…

users[id] = user;

…as the 3rd line, to achieve the same result. In fact, you can also make the 1st line more concise by just declaring the new User instance as follows:

User memory user;
user.balance = balance;
users[id] = user;

However, your Solution 2 still remains the most concise solution, no matter how much we can reduce the code in your Solution 1.

1 Like
#364 
function updateBalance(uint id, uint balance) public {
    // Bug: balance was assigned to the memory instance of the user struct
     //User memory user = users[id];
     //user.balance = balance;
     
    // Fix alt 1: 
    // the storage instance of the user struct is updated directly
    // seems convenient code when it is only one action on the user struct
    //  users[id].balance = balance;
    
    // Fix alt 2:
    // The user variable references the user struct in the storage instance
    // Seems overkill for one action on the struct, but looks like it could be adding clarity reading the code.
    // Easier to read code would help reducing risks mistakes if there are several actions on the user struct and maybe other users structs are handled at the same time.
     User storage user = users[id];
     user.balance = balance;
}
1 Like