Add ACL to near-token-factory #24

mfornet · 2022-10-20T12:36:51Z

No description provided.

mooori

Regarding the CI failure: It might be related to new of near-token-contract now having an input parameter.

near-token-contract/src/lib.rs

mooori · 2022-10-20T14:09:48Z

near-token-factory/src/lib.rs

+    /// Account id that will be used as admin for all deployed tokens.
+    token_admin: AccountId,


This probably requires extra care to not get out of sync with Contract.__acl which stores internal Acl state:

https://github.com/aurora-is-near/near-plugins/blob/a55c3a076d019af2202c295c4de4d0027dfc0c69/near-plugins-derive/src/access_controllable.rs#L56-L67

In particular, Acl allows multiple admins and grantees for AclRole::UpdateTokenAdmin, though Contract can have only a single token_admin. Could that lead to conflicts?

Is there any way to access super-admins? I couldn't find any way right now. Anyway, having an explicit super-admin might still make sense, since this will be for the individual contracts and not for the factory.

Good point, acl_get_super_admins is missing. I'll prepare a PR to add it to near-plugins.

As agreed in our discussion, renamed the field token_super_admin to reflect its intention and be aligned with the naming used in near-plugins. Fixed in 6dc7d48

near-token-factory/src/lib.rs

mooori

After #20 was merged this PR's base branch was changed from acl to main. Maybe that caused some issues with the diff (see comment below)?

Cargo.toml

near-token-contract/src/lib.rs

mooori

The diff makes sense after the rebase. I've left some comments below.

mooori · 2022-11-29T11:38:23Z

near-token-contract/src/lib.rs

+        // Grant MetadataUpdater role to the factory. This enables a trustless setup to set
+        // the metadata by any user. Super admin is responsible for adding a new user account
+        // with this role to provide icon and extra metadata.


Suggested change

// Grant MetadataUpdater role to the factory. This enables a trustless setup to set

// the metadata by any user. Super admin is responsible for adding a new user account

// with this role to provide icon and extra metadata.

// Grant MetadataUpdater role to the factory. This enables a trustless setup as

// described in [`Self::update_metadata`].

I think the description of update_metadata is a bit easier to follow than the comment here.

Anyway, maybe we want to describe the idea only in one place? Then there's no risk of two descriptions getting out of sync.

mooori · 2022-11-29T11:42:58Z

near-token-factory/src/lib.rs

 #[near_bindgen]
 #[derive(BorshSerialize, BorshDeserialize, PanicOnDefault)]
 pub struct Contract {
    /// Account id of the engine. It is expected to be `aurora`.
    aurora: AccountId,
+    /// Account id that will be used as super admin for all deployed tokens.


Suggested change

/// Account id that will be used as super admin for all deployed tokens.

/// Account id that will be used as super admin for newly deployed tokens.

Nit but I think all can be misleading since changing token_super_admin doesn't affect tokens that were already deployed (see doc comment of replace_token_admin).

mooori · 2022-11-29T11:46:21Z

near-token-factory/src/lib.rs

+        // Initialize Acl permissions.
+        require!(
+            contract.acl_init_super_admin(super_admin.clone()),
+            "Failed to add factory as initial acl super-admin",


Suggested change

"Failed to add factory as initial acl super-admin",

"Failed to initialize acl super admin",

It might be an account other than the factory if input parameter super_admin is Some(account_id).

mooori · 2022-11-29T11:49:47Z

near-token-factory/src/lib.rs

    /// Initializes the contract. The locker account id MUST be the NEAR
    /// representative of the Aurora address of the locker contract created
    /// using the Cross Contract Call interface.


Should the doc comment describe what's happening in terms of Acl?

mooori · 2022-11-29T12:00:45Z

near-token-factory/src/lib.rs

+        require!(contract
+            .acl_grant_role(AclRole::TokenBinaryUpdater.into(), super_admin.clone())
+            .unwrap_or_default());


Suggested change

require!(contract

.acl_grant_role(AclRole::TokenBinaryUpdater.into(), super_admin.clone())

.unwrap_or_default());

require!(

contract

.__acl

.grant_role_unchecked(AclRole::TokenBinaryUpdater, &super_admin),

"Failed to grant AclRole::TokenBinaryUpdater",

);

This call of acl_grant_role succeeds only if the contract itself is super admin or admin for TokenBinaryUpdater. Neither of it holds if input parameter super_admin is Some(other_account_id).

mooori · 2022-11-29T12:01:35Z

near-token-factory/src/lib.rs

+        require!(contract
+            .acl_grant_role(AclRole::TokenControllerUpdater.into(), super_admin)
+            .unwrap_or_default());


Same here, it should call grant_role_unchecked too.

mooori · 2022-11-29T12:06:08Z

near-token-factory/src/lib.rs

    /// so all deployed contracts SHOULD be upgraded after calling this function. ONLY the
    /// `Owner` role can call this method.


Suggested change

/// so all deployed contracts SHOULD be upgraded after calling this function. ONLY the

/// `Owner` role can call this method.

/// so all deployed contracts SHOULD be upgraded after calling this function.

///

/// Only accounts that are granted [`AclRole::TokenBinaryUpdater`] can

/// successfully call this method.

mooori · 2022-11-29T12:08:44Z

near-token-factory/src/lib.rs

+    /// Replace the account id that will be admin for all new tokens. This has no effect on
+    /// tokens that were already deployed.


Suggested change

/// Replace the account id that will be admin for all new tokens. This has no effect on

/// tokens that were already deployed.

/// Replace the account id that will be admin for all new tokens. This has no effect on

/// tokens that were already deployed.

///

/// Only accounts that are granted [`AclRole::TokenControllerUpdater`] can

/// successfully call this method.

mooori · 2022-11-29T12:42:56Z

near-token-factory/src/lib.rs

+                near_sdk::serde_json::json!({
+                    "super_admin": self.token_super_admin,
+                })


super_admin parameter of near-token-contract is Option<AccountId>:

https://github.com/aurora-is-near/native-erc20-connector/blob/78d504ed2f84120577fa5727c1ace2727b491028/near-token-contract/src/lib.rs#L81

Maybe serde is implicitly doing the conversion? If not, shouldn't passing AccountId instead of Option<AccountId> cause a test failure?

mooori · 2022-11-29T13:17:48Z

Should we have tests to verify Acl is integrated correctly? For example like this:

https://github.com/aurora-is-near/native-erc20-connector/blob/4b306c8152c73a1205cf3dea61f32433ff9e6aa2/tests/src/tests/mod.rs#L162

I would argue yes, since the Acl plugin is rather complex. Also these tests should catch things like using acl_grant_role when instead __acl.grant_role_unchecked is required.

mfornet requested a review from mooori October 20, 2022 12:36

mooori reviewed Oct 20, 2022

View reviewed changes

mooori mentioned this pull request Nov 21, 2022

Add acl to near-token-contract #20

Merged

Base automatically changed from acl to main November 22, 2022 22:23

mooori reviewed Nov 23, 2022

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

near-token-contract/src/lib.rs Show resolved Hide resolved

Marcelo Fornet added 3 commits November 28, 2022 21:08

Add ACL functions to factory contract

b6e3c04

User super-admin and assign roles properly

1506b5d

fix: Rename field to token_super_admin

78d504e

mfornet force-pushed the acl_factory branch from 6dc7d48 to 78d504e Compare November 28, 2022 20:14

mfornet requested a review from mooori November 28, 2022 20:19

mooori requested changes Nov 29, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ACL to near-token-factory #24

Add ACL to near-token-factory #24

mfornet commented Oct 20, 2022

mooori left a comment

mooori Oct 20, 2022

mfornet Oct 20, 2022

mooori Oct 21, 2022

mfornet Nov 23, 2022

mooori left a comment

mooori left a comment

mooori Nov 29, 2022

mooori Nov 29, 2022

mooori Nov 29, 2022

mooori Nov 29, 2022

mooori Nov 29, 2022

mooori Nov 29, 2022

mooori Nov 29, 2022

mooori Nov 29, 2022

mooori Nov 29, 2022

mooori commented Nov 29, 2022

		/// Account id that will be used as admin for all deployed tokens.
		token_admin: AccountId,

	/// Account id that will be used as super admin for all deployed tokens.
	/// Account id that will be used as super admin for newly deployed tokens.

	"Failed to add factory as initial acl super-admin",
	"Failed to initialize acl super admin",

		/// so all deployed contracts SHOULD be upgraded after calling this function. ONLY the
		/// `Owner` role can call this method.

		/// Replace the account id that will be admin for all new tokens. This has no effect on
		/// tokens that were already deployed.

Add ACL to near-token-factory #24

Are you sure you want to change the base?

Add ACL to near-token-factory #24

Conversation

mfornet commented Oct 20, 2022

mooori 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

mooori left a comment

Choose a reason for hiding this comment

mooori 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

Choose a reason for hiding this comment

mooori commented Nov 29, 2022