swivel-finance / gost

Smart contract testing with Geth via the Golang ABIGEN

Smart contract testing with Geth via the Golang ABIGEN

                _______                                 
               /______/\                                
               \__::::\/                                
      _______      ______       ______       _________  
     /______/\    /_____/\     /_____/\     /________/\ 
     \::::__\/__  \:::_ \ \    \::::_\/_    \__.::.__\/     ,--.
      \:\ /____/\  \:\ \ \ \    \:\/___/\      \::\ \      |  oo|
       \:\\_  _\/   \:\ \ \ \    \_::._\:\      \::\ \     |  ~~|  o  o  o  o  o  o  o  o  o  o  
        \:\_\ \ \    \:\_\ \ \     /____\:\      \::\ \    |/\/\|
         \_____\/     \_____\/     \_____\/       \__\/ 

Getting Started

This project contains the Swivel Smart Contracts and Libraries which have been compiled to their abi and bin components, those then transformed into golang bindings via the Geth abigen tool. You can see the commands used to perform these tasks in the Makefile.

Tests are located in /pkg/*testing/. For example, the unit tests for the Swivel.sol contract will be located in /pkg/swiveltesting.

Existing tests, and any newly created, are always run via go test [-v] ./... from the project root.

If you are only here to view the existing tests, and maybe pull this repo and run it you can stop here. If you are a developer writing new tests or simply an interested party who realizes the mind blowing superiority of testing your smart contracts this way - read on!

Notes on /build

The /build directory exists to house our 2 deployed contracts (and 2 deployable child contracts) with all of their dependencies and compiled artifacts. Those being not only the .sol files but the .bin, .abi and .go files as well. These compiled artifacts will be different from what is in the /test directories, as those are compiled with mocks in place (these are not).

Note that the files here should be considered "alpha" stage as what is present here should be considered "stable".

Geth

Gost only depends on Geth itself. We do list solc as being necessary, but that isn't exactly true. If you are in possesion of the abi and bin files of any smart contract (regardless of language used) the Makefile's compile_* type steps can be made for them.

As this Golang project is done with modules, you don't need to specifically go get anything. All dependencies will be fetched on your first test run.

Solidity Compiler

First, assure that solc is in your $PATH. If not, make it so. As stated above, this is assuming your smart contracts are .sol and you do not already have the .abi and .bin files. Add new Makefile rules for other languages (Vyper for example) and compilers if you are so inclined.

Geth ABIGEN

Second, you will need abigen in your $PATH. You can, for example, follow the instructions here. Note that you only need to use the devtools rule from the go-ethereum Makefile. i.e. make devtools will suffice.

Project Structure

As always we use the project layout guidelines. This interpretation places the Solidity files into test/ with their compiled artifacts going into aptly named subdirectories for ease of use with golang's package conventions (see Makefile). Some notes on the important residents of /test/:

• swivel/
  • Swivel.sol. The current Swivel smart contract
  • Abstracts.sol. The external interfaces declared by Swivel.sol
  • Sig.sol. Imbeddable library used by Swivel.sol
  • Hash.sol. Imbeddable library used by Swivel.sol
  • swivel.go. Autogenerated Geth bindings for use in tests.
• marketplace/
  • MarketPlace.sol The current MarketPlace smart contract
  • Abstracts.sol. The external interfaces declared by MarketPlace.sol
  • VaultTracker.sol. A mock implementation of the VaultTracker smart contract. Used to build and test MarketPlace in isolation
  • ZcToken.sol. A mock implementation of the ZcToken smart contract.
  • marketplace.go. Autogenerated Geth bindings.
• vaulttracker/
  • VaultTracker.sol. The current VaultTracker smart contract
• tokens/
  • ZcToken.sol. An Open Zeppelin clone of an extended Erc20 with mint, burn and permit.

The Makefile

This is the way.

Steps for setting-up, tearnig-down and testing the individual pieces of the repo are here.

Compiling and Testing Your Contracts

• Compiling:
  • make all

If you wish to run the steps separately or run into any errors that need debugging, see the Makefile for the entire list of available commands.

• Testing:
  • go test ./... from root (as stated). Add the -v flag if you expect to see any logging you may be doing.
  • You can of course test at the package level: go test ./path/to/package

gost's Issues

setup go environment. pull project. run tests

title.

SSF-02

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L12

ZERO_ADDRESS check(s)

likely won't fix

MSF-01

https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L14

const and immutable for vars.

will fix.

SSF-05

about ERC20 not returning bools from transfer / from again.

SafeErc20 lib recommended

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L78

MSF-06

return values from ERC20 on transfer / transferFrom

https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L104

is this issue saying that not all Erc's return bool here? looks like they suggest the OZ lib SafeErc20

something to look into, particularly if we do not control what that Erc20 is... more quetions...

FillFixed implemented

complete with tests.

MSF-05

contract size of the Marketplace

they are obvs not looking at our bins, which are run with the suggested fix already

i documented this thoroughly already:

https://github.com/Swivel-Finance/gost/blob/v2/Makefile#L209

anything below the used number did not change the size of the deployed code at all.

Test balancesOf in VaultTracker

Cancel refactor and test

Update README

v2 has some changes to structure.

Generate Underlying bindings

• add name() for Underlying and generate bindings

SSF-10

put err messages in the loop requires

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L49

likely will fix

SSF-08

here we do require on approve, but that is the recurring "use SafeErc20" issue

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L84

VaultTracker initial testing

Setup and start to "fill" VaultTracker contract.

Implement tests:

• constructor test
• AddNotional test

Implement contract deployment scripts.

Test MarketPlace redeemZcToken method

ReleaseFixed refactored and tested

batchRelease refactor and test

ERR-01

https://github.com/Swivel-Finance/gost/blob/v2/test/tokens/ERC2612.sol#L47

i suggest googling around to find the state of this argument with folks are are actively using the "permissioned token".
as with the ERE erros, this is O.Z code.

VaultTracker tests

Implement tests for the following methods:

• addNotional
• removeNotional
• matureVault
• redeemInterest
• transfer

batchFill refactor and test

ERR-02

https://github.com/Swivel-Finance/gost/blob/v2/test/tokens/ERC2612.sol#L46

forks, chain'd and DOMAINs oh my!

i think the discussion here is:

• is this a realistic attack scenario
• in moving the DOMAIN calc to the permit func worth it for this possible attack

Add cTokens mapping

When we have more than 1 underlying asset being transacted, we will need to store a number of address -> address relationships for each underlying/ctoken pairing.

  ///@dev maps a cToken address to its corrosponding underlying token address
  mapping (address => address) public cTokens;


  ///@notice allows admin to enable cToken pairs
  ///@param underlying : address of the underlying token being paired
  ///@param cToken : address of the corrosponding cToken for the underlying provided
  function pairTokens(address underlying, address cToken) public {
      require(msg.sender == admin, "Only Admin");
      cTokens[underlying] = cToken;
  }

This implemented along with adding admin = msg.sender to the constructor allows the address that deployed the contract to create these pairings which can then be accessed for any contract functionality.

SSF-01

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L12

const and immutable for the one time vars

likely will fix

ERE-02

https://github.com/Swivel-Finance/gost/blob/v2/test/tokens/ERC20.sol#L43

defaut

very important...

SSF-03

boolean const vs val again. what are they saying?

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L47

SUS-01

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Sig.sol#L30

this is the cryptography vrs thing again... wut?

implement and test `fillFloating`

Test MarketPlace redeemVaultInterest method

ERE-01

https://github.com/Swivel-Finance/gost/blob/v2/test/tokens/ERC20.sol#L36

This contract and these variable are open zeppelin code.

If we want to modify this, we should discuss all the changes to O.Z code in total as it's own set of tickets.

NOTE: we do already remove their "context" component, so it's not out of the question...

SSF-04

re-entrancy again

essentially move execution to post checks

likely will fix

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L47

Implement VaultTracker TransferNotionalFrom

Implement VaultTracker TransferNotionalFrom instead of calling both removeNotionalFrom and addNotionalFrom methods. Change tests, as well.

ERE-03

https://github.com/Swivel-Finance/gost/blob/v2/test/tokens/ERC20.sol#L37

This is more O.Z. code and falls under that umbrella

MSF-07

https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L104

looks like the suggestion it to assure a is > 0 to throw earlier than it would throw anyway?
is this actually an improvement?

Test MarketPlace matureMarket method

VTS-01

const and immutable keywords on the one time vars again

likely will fix

https://github.com/Swivel-Finance/gost/blob/v2/test/vaulttracker/VaultTracker.sol#L8

SSF-09

check the length of the input arrays

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L44

maybe... no strong feelings

this is in the -- its gunna throw either way - do we need moar code as to be nicer... prob so but worth asking...

Todo: Vault balancesOf vs. balanceOf (Notional)

Currently, VaultTracker has a balancesOf function that returns (notional, redeemable).

This makes sense at first but in context of its use we might want to just leave it as balanceOf, returning (notional).

User Story 1:
Checking BalanceOf for the Notional only.
In most cases it makes sense to check balanceOf in isolation.
E.g. balance check for transfer, balance check for orderbook solvency, etc.,

User Story 2:
Checking the amount of interest currently redeemable.
When checking for the amount of interest currently redeemable, we first read the redeemable balance. However this redeemable is based on the last time at which a user interacted with our contracts, not on the current moment in time.

That said, in order to accommodate the user story, one must read all of vaults[address].

Once reading exchangeRate, one needs to then take the current cToken exchangeRate and identify the % difference between the two. From there you can apply that % difference to notional to get the marginal interest generated since a previous contract interaction. Add that to redeemable to get the true currently redeemable interest.

All this considered, it might make sense to change the balancesOf function and solely return balanceOf notional in order to accommodate the first user story given balancesOf fails to properly fulfill the second user story anywho (doesnt return exchangeRate).

The big mock pattern...

So this is a change that needs to happen in the 2 mock tokens as, while the previous terms worked in v1, they dont "scale" out to v2 -- but this new pattern will work for all (does "scale")...

look at the /mocks/MarketPlace.sol

there is a pattern for the many names of things these mocks have. It is based around the method name. here is a contrived example

function fooBar(arg1, arg2) whatever returns (bool); // note the bool return here - it matters

this means the following:

• the mock should have a state variable fooBarReturn (note no 's' on the end)
• the mock should have a setter for the above state var named fooBarReturns (note the 's')
• the mock should have a mapping (arg1Type => arg2Type) named fooBarCalled (note the method only had 2 args)

NOTE: if any method return is not a bool, but rather some state variable, the only thing that changes is that you can use the actual state var's name in place of fooBarReturn. the setter fooBarReturns is unchanged as well as everything else.

now, another example where the method has more than 2 args

function barBaz(arg1, arg2, arg3) whatever returns (bool)

meaning:

• a struct must be made BarBazArgs

  struct BarBazArgs {
  arg2Type: label
  arg3Type: label
  }

• the mock should then have a mapping (arg1Type => BarBazArgs)

• the mock has a state var barBazReturn

• the mock has a setter barBazReturns

in both cases the mappings allow us to reflect over what args the contract sent,
and the setters allow us to control what the methods reply with.

the change that needs to occur is that the /mock/Erc20 and /mocks/CErc20 used a different naming convention where the mehod name was "past tensed" in place of fooCalled.

EX in /mocks/Erc20

• TransferFromArgs is correct
• TransferredFromArgs is incorrect and should be changed to TransferFromCalled
• TransferredArgs is also incorrect and should be TransferCalled

Event index discussion

We should talk about all the events in the protocol and what event indexes will be.

MarketPlace p2p & custodial methods

This is a master ticket for implementing and adding the following MarketPlace methods:

• custodialExit
• custodialInitiate
• p2pZcTokenExchange
• p2pVaultExchange

MSF-02

check for ZERO_ADDRESS on passed in address args

will fix

NOTE. this, to me, does not include the onlyAdmin and onlySwivel checks.

Feature Request: functions splitUnderlying(amount), combineUnderlying(amount)

With the context that there are potential market dynamics where a user might split principal into notional and zcTokens, it makes sense to add these two functions.

splitUnderlying:
Input: amount
Takes an amount of underlying currency, deposits it into swivel (and compound), mints the user the same amount of zcTokens and vault notional.

combineUnderlying:
Input: amount
Redeems an amount of currency by combining zcTokens and vault notional. In the process the amount of underlying is withdrawn from compound and then from Swivel and the same amount of zcTokens and notional are burnt.
naming: combineUnderlying makes sense enough and I'm happy with it. comineZcTokenAndNotional would be more accurate but this gets the point accross I think? Maybe combineForUnderlying?

This covers more generalized functions for users that want to market-make or trade around + facilitates some secondary market usage.

SSF-06

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L78

a being > 0 check again
not sure this is an improvement still...

MSF-04

https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L68

re-entrancy possibility. looks like suggestion is to move ln.74 to the end of the function

likely will fix

ReleaseFloating refactored and tested

SUS-02

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Sig.sol#L49

redundant return

will fix but not theirs as our style is not to use the named returns in the return statement

Remove memory vars from VaultTracker.sol

We're fetching vaults from the mapping and setting properties to them. When we're done with it we need to store them again in the mapping - weird, I know.

We should get rid of memory vars and use the vault directly.

Example:

Vault memory vault = vaults[o];
vault.foo = bar;
vaults[o] = vault;

can become

vaults[o].foo = bar;

SSF-07

https://github.com/Swivel-Finance/gost/blob/v2/test/swivel/Swivel.sol#L115

require on the approve call

likely will fix

note that would incur the same prob as the recurring "use safe ERC20" issues (also #115)

MSF-03

https://github.com/Swivel-Finance/gost/blob/v2/test/marketplace/MarketPlace.sol#L69

comparing bool vars (values to const)

what is the suggested solution?