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

Add accType attribute for Accounts #491

Conversation

KelvinThai
Copy link
Contributor

@KelvinThai KelvinThai commented Apr 7, 2023

Description

This PR add, AccType attribute for AccountID
Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist before requesting a review

  • I have performed a self-review of my code.

Deployment

  • Should publish the npm package

@KelvinThai KelvinThai linked an issue Apr 7, 2023 that may be closed by this pull request
@KelvinThai KelvinThai changed the title added accType variable for Account struct and AccountCreated event added accType attribute for Accounts Apr 7, 2023
@KelvinThai KelvinThai self-assigned this Apr 7, 2023
@KelvinThai KelvinThai marked this pull request as ready for review April 7, 2023 03:30
@@ -150,14 +151,21 @@ contract Prepayment is Ownable, PrepaymentInterface, TypeAndVersionInterface {
s_currentAccId++;
uint64 currentAccId = s_currentAccId;
address[] memory consumers = new address[](0);
s_accounts[currentAccId] = Account({balance: 0, reqCount: 0});
string memory accType = "regular";
Copy link
Member

Choose a reason for hiding this comment

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

@KelvinThai Thank you for digging in to the problem! I also thought about this. I will try to explain it and let me know what you think.

We could create an extra mapping to store registered coordinators instead of looping through all coordinators which punishes regular accounts creators by increasing gas cost for them.

mapping(address => bool) private sIsCoordinator;

The values of sIsCoordinator will be updated inside of addCoordinator and removeCoordinator.

The createAccount function will then look similar to the one below.

function createAccount() external returns (uint64) {
    s_currentAccId++;
    uint64 currentAccId = s_currentAccId;
    address[] memory consumers = new address[](0);
    string memory accType = "reg";
    if (sIsCoordinator[msg.sender]) {
        accType = "tmp";
    }
    s_accounts[currentAccId] = Account({balance: 0, reqCount: 0, accType: accType});
    s_accountConfigs[currentAccId] = AccountConfig({
        owner: msg.sender,
        requestedOwner: address(0),
        consumers: consumers
    });

    emit AccountCreated(currentAccId, msg.sender, accType);
    return currentAccId;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agreed. A mapping would give faster and cheaper access to Coordinators. Let's move this way

@KelvinThai KelvinThai requested review from a team and bayram98 as code owners April 7, 2023 08:02
Copy link
Member

@martinkersner martinkersner left a comment

Choose a reason for hiding this comment

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

@KelvinThai LGTM! I added just one small suggestion, but it is ready to be merged!

@@ -393,6 +406,7 @@ contract Prepayment is Ownable, PrepaymentInterface, TypeAndVersionInterface {
}
}
s_coordinators.push(CoordinatorBaseInterface(coordinator));
sIsCoordinators[coordinator] = true;
Copy link
Member

Choose a reason for hiding this comment

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

Since we introduced the mapping which holds all active coordinators, we can simplify this function a lot. It could look as in the listing below.

function addCoordinator(address coordinator) public onlyOwner {
    if (sIsCoordinators[coordinator]) {
        revert CoordinatorExists();
    }
    s_coordinators.push(CoordinatorBaseInterface(coordinator));
    sIsCoordinators[coordinator] = true;
}

Copy link
Member

@martinkersner martinkersner left a comment

Choose a reason for hiding this comment

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

LGTM!

@martinkersner martinkersner merged commit 401aad4 into master Apr 8, 2023
@martinkersner martinkersner deleted the 364-separate-counter-for-direct-payment-and-prepayment-acount branch April 8, 2023 07:21
@martinkersner martinkersner changed the title added accType attribute for Accounts Add accType attribute for Accounts Apr 8, 2023
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.

Separate counter for direct payment and prepayment acount
2 participants