[FeedBack] Student hoping for feedback on a few contracts

[jotaAfonso]

Hello, I'm a student currently doing a thesis, involving smart contracts, and I would like to receive some feedback on a few contracts that I made.

Contracts:

• https://github.com/jotaAfonso/Smart_Contracts/tree/main/scilla

The contracts follow a state machine approach.
The automaton that corresponds to the logic of the contract is in the README file.

There are a few of them, but I would like to highlight two specific contracts:

• the bazaar because it is a contract that interacts with another contract;
• and the auction because there is more logic than the rest of the other contracts.

Looking for feedback on glaring errors and if there are some improvements to be made.

Thank you in advance and best wishes.

[jubnzv]

Hi João,

Cool! What your thesis is about? Will it be available somewhere?

I took a look at these contracts. I think that's a good start. I see the working Scilla code, but I have a few questions about logic that this code implements.

But first, I'd like to give a small recommendation, that could improve your development experience. Take a look at the scilla-checker. It is a static analyzer for Scilla, which can found some errors before deploying it to the network. It is integrated in the Neo Savant IDE. Otherwise, you could run it locally. A command to check a contract may look like: scilla-checker -gaslimit 8000 -libdir ~/Dev/scilla/src/stdlib Auction.scilla.

And here are my initial thoughts on the code. Let's start with that, maybe later we could go further.

Auction.scilla

1. The Withdraw transition. I found, that your implementation follows the Pull over Push pattern what is a good idea. But I found the following lines suspicious:

l_Result_State = Start;
state := l_Result_State;

Consider the following situation. We have a few pending returns, say, pendingReturns = { A, B, C }. Now, A calls the Withdraw transition and sets the state field to the Start value. Because of this, B and C cannot return their bids later (see this line).
The possible solution is to check that there are no pending returns before setting the Start state.

2. What the Bid state is used for? Let's take a look at the Bid function. Its comment says that the bid is possible if the auction wasn't ended. But in the current implementation it works in the Start state only (because of this line). Is this intentional?

3. I suggest to write a function RequireOwner to simplify owner check here:

procedure RequireOwner(address: ByStr20)
  is_owner = builtin eq _sender address;
  match is_owner with
  | True => (* Noop *)
  | False =>
    error = NotOwner; (* Let's add a new error for that case. *)
    ThrowError error
  end
end

4. Running scilla-checker on the contract raised a few warnings, that look reasonable:

Auction.scilla:7:8: warning: [3] Unused imported library: BoolUtils
Auction.scilla:51:9: warning: [3] Unreachable pattern AuctionStillLive
Auction.scilla:29:3: warning: [3] Unused ADT constructor: AuctionStillLive
Auction.scilla:224:9: warning: [3] Consider using "AddFunds" tag instead of empty tag as suggested by ZRC-5
Auction.scilla:194:7: warning: [3] Consider using "AddFunds" tag instead of empty tag as suggested by ZRC-5

The source of aforementioned ZRC-5 is present here: ZRC/zrc-5.md at main · Zilliqa/ZRC. For the more context about that error see this issue.

5. What is the life cycle of the contract? It is assumed, that if the owner ended auction, it won't be possible to start it again later? When all the bidders take their refunds back.

Bazaar.scilla

1. The Bazaar contract itself cannot be executed because of the small syntactic error (extra semicolon). And both contracts have a few scilla-checker warnings:

Bazaar.scilla:33:7: warning: [3] Unused field: bazaar_maintainer
Bazaar.scilla:31:7: warning: [3] Unused field: party_B
Bazaar.scilla:30:7: warning: [3] Unused field: party_A
Bazaar.scilla:42:9: warning: [3] Write only field: state
Bazaar.scilla:4:27: warning: [3] Unused imported library: ListUtils
Bazaar.scilla:4:18: warning: [3] Unused imported library: IntUtils
Bazaar.scilla:8:5: warning: [3] Unused library value: zero_address
Bazaar.scilla:11:5: warning: [3] Unused library value: success_code
Bazaar.scilla:16:3: warning: [3] Unused ADT constructor: Finalized
ItemListing.scilla:52:9: warning: [3] Write only field: price
ItemListing.scilla:61:9: warning: [3] Write only field: parent_Contract
ItemListing.scilla:51:9: warning: [3] Write only field: name
ItemListing.scilla:3:27: warning: [3] Unused imported library: ListUtils
ItemListing.scilla:25:5: warning: [3] Unused library value: two_msgs
ItemListing.scilla:107:11: warning: [3] Consider using "AddFunds" tag instead of empty tag as suggested by ZRC-5
ItemListing.scilla:103:11: warning: [3] Consider using "AddFunds" tag instead of empty tag as suggested by ZRC-5

2. The contract needs an informal description, maybe present as comments in the source code. As a reviewer, it is not easy to understand what is going on, when you're reading it for the first time.

3. We could extract procedures from pattern matching statements that check some conditions. For example, we could create a IsPriceValid procedure from this match or IsSellerEither from these lines.

[jotaAfonso]

Hello, thank you for the reply,

The thesis is about "Smart Types for Smart Contracts Validation," in which the automaton of the Smart Contract plays a big part, which is something that I didn't make clear in my first request.

Regarding where the thesis will be available, I'm still elaborating it, so I will try to post it here when it's concluded.

1. Notes (Regarding both contracts):

• I used the scilla-checker and didn't get the warnings (are warnings exclusive to a local environment?).
• Added context to the contracts, including their description and the possible transitions, before the keyword contract;
• Instead of looking at the automaton, I provide, in a written form indicating for each state,
  the possible transition that the contract can execute with that state and indicating the resulting state of executing that transition.

2. Notes (Auction):

• I addressed all of the points, but regarding point 5, the life cycle would be after withdrawing any remaining pending returns or closing the auction if there are no pending returns. Is there a need to add anything else?

3. Notes (Bazaar):

• The context was added to the contract, transitions and several steps.
• Errors and events were added, and a few procedures were made so that the contract is more in line with the logic of the Auction contract.

I would appreciate, if you could look at it once again.
And is the list_Item transition in Bazaar.scilla comunicating well with the ItemListing.scilla contract?