Modular contracts for easy upgrades

[ngrinkevich]

• Add Staking Proxy Contract.
• Make old contracts compatible with staking proxy
• Reformat contracts with 4 spaces
• Update tests

[Shadowfiend]

We really need to split this up into more atomic commits. Please see https://www.freshconsulting.com/atomic-commits/ for more. One commit with +608/-429 is almost certainly too big, unless it's reformatting a whole file (which is usually a sign we need to add some basic formatting helpers). Here, we're not just reformatting, but we're reformatting and doing other work in the same commit, making it much harder to distinguish what the substantive changes are, and making it harder to navigate file history when trying to discover e.g. where a bug was introduced.

Separately, what triggered the 4-space reformat?

[ngrinkevich]

@Shadowfiend adding the new contract triggered the reformatting and I got tired seeing linter complaining over spaces :)
yeah let me do the formatting PR first, it will help @pschlump as well I think

[pschlump]

For the Go interface I also need a 2nd Event StakedAny (or some more appropriate name) that leaves out the "indexed" on the "user"

event StakedAny(address user, uint256 amount)

The abigen Go code will not let me catch the Staked event (with address indexed user unless I know the address of the user. My understanding of the higher level code means that it will never know the address of the user until the event happens. This is a circular loop. (((If you find a way to get the Go code to listen for events like this w/o removing the "indexed" I am totally for just the one event)))

[Shadowfiend]

The key problem here is the indexed, because it's used for filtering, right? My reading of the eth_newFilter docs seems to indicate we should be able to ignore the indexed stuff (topics) with the right parameters, no? Or is it just that the generated Go code can't specify a null for the topic?

[pschlump]

I have tried using NULL for the filter and I never get back the event. Inside geth the bloom filter is different when I use NULL than if I setup the same test from node.js - This may indicate that I am not doing it right - or that there is a defect in Geth or Abigen.

[pschlump]

The 2nd Event could just be added at this point. There are some tradeoffs. Is it better to have the proxy think and not do any work - or better to guarantee that both events always happen?

[pschlump]

I ran a bunch of tests on this and it works. Very impressive code!

Tests: I installed the contracts and verified that I could generate staked and events.
Then I checked that the staked balances were correct. The I updated the contract to a new
contract ( I added a method that just returned a constant so I could verify the new contract)
Then checked that the staked balances were still correct. Then added new staked and verified that I was still getting the event - then used the address of the old version of the contract to verify that it had not changed and used the address of the new version to verify that it had changed the balances.

This all worked as it should.

[ngrinkevich]

@keep-network/contracts hey guys, need your thoughts on this pls

Initially I was thinking having activeContract on the proxy is a good idea but then plugin in tokenGrant contract becoming complex as technically it's also active contract.

What if we simplify proxy to this:

• Just a list of authorised staking contracts (Owner manages them)
• Broadcast Stake/Unstake originated from listed contracts (Child contracts call those on the proxy)
• Method to get total of staking balances from listed contracts

and leave end user (dApps) to do stakes/unstake directly on the child contracts since they might also have different implementation for unstake (currently 2 step initiateUnstake/finishUnstake) We also might upgrade child contracts to match similar interface ethereum/EIPs#900 so even with that we won't need to change our proxy contract

[Shadowfiend]

One thing that makes me a bit squirmy is the bidirectional link from proxy to active contract and back. I don't have a specific concern off the top of my head but that feels like it could yield future pain... Need to think about it some more.

Is the fundamental concern you're trying to allay is that the proxy contract is locked in in terms of interface, or something else? My current sentiment is that we should still expose our complete basic interface. Transition to a new interface will require a bit of work, but can be managed by (a) introducing a new active contract that's compatible with the current and future interface, (b) introducing a new proxy for the future interface, (c) transitioning people between the two proxy contracts.

One aspect of having stake/unstake happen on the underlying active contract is that it makes developing automated stake/unstake tools harder (that may be desirable, not sure yet). It'll also require additional security in the proxy contract, since the proxy contract needs to ensure that a contract trying to emit Stake/Unstake is in fact the active contract.

[Shadowfiend]

Btw, good to get into the habit of using @keep-network/contracts if you want eyes on contracts stuff :)

[ngrinkevich]

Is the fundamental concern you're trying to allay is that the proxy contract is locked in in terms of interface, or something else?

@Shadowfiend I think my concern is the concept of "active contract" I want to get rid of it :) since it complicates things

[Shadowfiend]

We'd still need that concept for fetching things like getting total balances though, no? Moreover this leaves us in the old situation of not being able to rely on a deployed change to the contract being seen by the Go clients unless the clients are updated. If I understand the situation correctly, I think it sacrifices too much.

[ngrinkevich]

@Shadowfiend ah no, I think we misunderstood each other :)

I just want to remove "activeContract" from the proxy contract line 26 and have just a list of authorised staking contracts

all of your concerns below are still resolved

We'd still need that concept for fetching things like getting total balances though, no? Moreover this leaves us in the old situation of not being able to rely on a deployed change to the contract being seen by the Go clients unless the clients are updated.

Go clients deal with just 1 address (Staking Proxy contract) to get total staking balances and listen for Stake/Unstake events

[Shadowfiend]

I think I've not followed fully then. What is the difference between activeContract and the authorized contracts?

Also keep in mind that we may want folks to be able to stake/unstake from the Go client (we're dealing with developers, after all).

[ngrinkevich]

What is the difference between activeContract and the authorized contracts?

I think simply put activeContract is just one contract and authorized contracts is a list of active contracts

Also keep in mind that we may want folks to be able to stake/unstake from the Go client (we're dealing with developers, after all).

In that case they get a list of authorised contracts, check the balances and stake/unstake on them directly

[ngrinkevich]

@Shadowfiend @pschlump I've created a new PR with cleaner commits, pls lets switch there, also @pschlump lets investigate if we can make just one event work without duplicating indexed/non indexed

[Shadowfiend]

Kk, moving the convo there and closing this PR.