Notes and feedback (first pass)

[MarvinJanssen]

My feedback on BNS-V2. I did a first pass on general structure, design principles, and so on. I'm happy to do a more in depth audit / stress test the coming week or whenever the next version is ready.

Some of these items express my personal preference of how I would do certain things, or inclusions that I see as beneficial to the ecosystem and users of the contract. We can split out separate issues of those items that you see as valid and the rest we can strike off. I turned it into a task list so it can easily be turned into a separate issue.

token-uri

• [x] token-uri is a variable without a setting. Change to constant or introduce update mechanism.

Transfer function

• [x] 1. I suggest to move manager transfer functionality to a separate function. It makes the function simpler.
• [x] 2. Ownership check should be (or (is-eq tx-sender nft-current-owner) (is-eq contract-caller nft-current-owner)) to make it a lot safer for contract principals to transfer names they own.

list-in-utsx functions

• [ ] I personally never liked to have market logic inside NFT contracts but it is a personal preference. Makes more sense to me to have it in a separate contract.

set-primary name / mng-burn / mng-manager-transfer

• [x] Most let variables are only used once, can inline to save on write dimension.

Management transfers

• [x] I think there should be an explicit property that freezes the manager property. It will make it safer and clearer for users that buy names in the namespace what managers can do with the names.

Preorder duplicate checks

• [x] Salt should be random, so in that case you will never detect that a user already preordered the same name before.

Preorder STX burning

• [x] It feels unfair to burn tokens on pre-order. Multiple people might pre-order the same name but only the first actually gets the name. Everyone else just loses STX. Transfer the STX to the contract and burn only on reveal, then allow people to claim the invalid preorders back. The only tricky bit is names that are moved later due to preorder ordering. But this than nothing...

namespace-reveal

• [x] Add a parameter for can-update-price-function so that users can see it and managers can freeze the prices from the beginning.

name-register

• [x] 1. This function is highly complex. I suggest to cut down on nesting and even to split the function into multiple functions if it cannot be simplified. (But it can.)

• [ ] 2. I suggest to remove the following:

;; Verifies that 1 block has passed from when the preorder was made
(asserts! (> block-height (+ (get created-at preorder) u1)) ERR-NAME-NOT-CLAIMABLE-YET)

The reason is that it is a user or UI mistake if both transactions are in the mempool at the same time. Thus, why arbitrarily prevent the user from registering the name? As soon as both transactions are in the mempool the name becomes snipable. Adding this line ensures that the actual user will never be able to buy the name before the sniper. Without the line, the user has a chance if their transaction is processed before the sniper transaction.

• [x] 3. I would change this line and also add a check for contract-caller to be more contract owner friendly:

(asserts! (not (is-eq tx-sender (get owner name-props-exist))) ERR-OWNER-IS-THE-SAME)

4. I suggest returning id-to-be-minted from the function as it makes it a lot easier for other contracts to build on top of this contract.

"fully-qualified-name"

• [x] 1. The thing called "fully-qualified-name" is not a fully qualified name, it is a hash. I suggest to rename that property everywhere. If it is supposed to be a FQN then there is a programming error.

• [ ] 2. I also do not really understand the purpose of the FQN hash. It contains a random salt so you will never detect collisions anyway. Is it just to be on the safe side? I don't think it will work the way you think it works.

purchase-transfer

• [x] 1. The comments says that the function is "SIP-09 compliant function" but it is not. No definition for purchase-transfer exists in the SIP9 standard.

• [x] 2. Almost all let vars are unused. Remove them and inline.

• [ ] 3. The function will fail if the owner and recipient are the same. That code path is accessible in places.

The linked list

• [x] The linked list is cool but what is the point to having it on-chain? It adds a lot of complexity and I do not see any getters for it. Is it purely for the primary names? Might be able to manage that in different ways. Indexing who owns which names is trivial off-chain.

update-zonefile-hash

• [x] I think there is a fundamental design flaw here. It looks like if a namespace is managed, then owners of names cannot update their own zonefile hashes. Is that intentional? Do users then update the zonefile through the manager? I do not understand why it would be limited to only the manager. Should the owner not always (only) be able to update the zonefile hash of the names they own?

name-revoke

• [ ] 1. Same as above. Looks like users cannot revoke their own names if it is a managed namespace. Is that intentional?

• [ ] 2. It looks pretty scary that managers can revoke any name at any time. It would be nice if there is a flag in namespace-props that disables manager revokation rights.

name-renewal

• [x] This function is highly complex with many nested conditional structures. I suggest to flatten by using assertions.

Name pricing

• [ ] Is that whole name pricing algorithm still necessary? The vowel and non-alpha discounts.

• [ ] Should the managers not have pricing mechanisms instead?

[Patotking12]

2. I also do not really understand the purpose of the FQN hash. It contains a random salt so you will never detect collisions anyway. Is it just to be on the safe side? I don't think it will work the way you think it works.

For this one, it is not to detect collisions, it is to store the best preorder information, so that when doing validation when registering a name we have the preorder used to register the name previously so we can compare preorder's information, and we keep or update to the one that is the right preorder for the name.

[Patotking12]

3. The function will fail if the owner and recipient are the same. That code path is accessible in places.

Here the purchase-transfer function: used on handle-existing-name (new private function) that transfers an existing name that was "wrongfully" registered by someone else, so the owner and recipient is not the same. On handle-renewal-after-grace-period (new-private-function) that only executes if the renewal is not made by the current owner, so owner and recipient will not be the same. On buy-in-ustx which is the only place if someone buys their own listed name, that that collision will happen

[Patotking12]

1. Same as above. Looks like users cannot revoke their own names if it is a managed namespace. Is that intentional?

2. It looks pretty scary that managers can revoke any name at any time. It would be nice if there is a flag in namespace-props that disables manager revokation rights.

En the previous version, owners were not able to revoke their own name, only the import-address was able to revoke names, in this case we are only adding that ability to managers too.