refactored, fixed bugs and optimized smart contract #1
Conversation
|
I've also forgot to mention that anyone could approve a spender for shares of a contract. I've added a check to prevent that |
| address spender, | ||
| uint _index, | ||
| uint amount | ||
| ) external exist(_index) onlyPropertyOwner(_index) { |
There was a problem hiding this comment.
This function can be removed entirely because it's not used.
In the end I decided to let the smart contract of the marketplace to own the House Tokens.
|
|
||
| /// @dev checks if a property exist | ||
| modifier exist(uint _index) { | ||
| require(exists[_index], "Query of non existent property"); |
There was a problem hiding this comment.
nothing is passed to the exist variable in this PR so maybe it would be better to check if the index is in the properties mapping? or am I missing something?
There was a problem hiding this comment.
Ok i just sure the exist function but not sure if I agree/understand that choice for the implementation, would love to hear the rationale
There was a problem hiding this comment.
Oh I'm sorry, I forgot to add the related line when writing a property. In your case, your functions have enough checks to prevent the bugs I could find that were related to that, but the transactions would be reverted with a misleading reason.
| _; | ||
| } | ||
| /// @dev checks if any property's shares have been sold | ||
| modifier onlySharesNotSold(uint _index) { |
There was a problem hiding this comment.
great idea to make these modifiers
| public | ||
| view | ||
| returns (uint){ | ||
| function getPropertiesLength() public view returns (uint) { |
There was a problem hiding this comment.
there are some times you change the function signatures to have one item per line and then in this case it's been compressed to one line. What's the reason?
There was a problem hiding this comment.
It's my formatting extension (I believe it's currently set on the hardhat + solidity)
| view | ||
| returns(bool) | ||
| /// @dev returns the number of shares available for sale | ||
| function getPropertyTokensRemaining(uint _index) |
There was a problem hiding this comment.
yes this is a useful function to have when the code is extended, thank you
| require(msg.sender==properties[_index].owner, "Only the property owner can cancel the sale"); | ||
| require(properties[_index].stockData.sold==0, "The sale cannot be updated if some tokens are already issued"); | ||
| require(properties[_index].status!=Status.SaleCancelled, "The property sale is cancelled"); | ||
| require(_price > 0); |
| { | ||
| require( | ||
| properties[_index].owner != msg.sender, | ||
| "You can't buy shares off your own property" |
There was a problem hiding this comment.
haha i don't mind if they do buy shares of their own property, they can be a part owner with other folks. that's ok
2 participants
Fixes and Improvement