Update Pool Box

[SethDusek]

Hi, here's some initial work for #49.

I have some questions.

1. Is there any case where a user might want to update the reward tokens used in the oracle, without making any other changes (new refresh NFT, etc)? Because right now, the update contract requires that the new pool's script be different from the old one, so simply replacing say old reward tokens with a new token, or adding more tokens to the pool is not possible.
2. Please check my comment in test_update_pool_box commit

[greenhat]

Hi, here's some initial work for #49.

Awesome!

I have some questions.

1. Is there any case where a user might want to update the reward tokens used in the oracle, without making any other changes (new refresh NFT, etc)? Because right now, the update contract requires that the new pool's script be different from the old one, so simply replacing say old reward tokens with a new token, or adding more tokens to the pool is not possible.

Good question! I think it's a legit case. I asked @scalahub in https://github.com/ergoplatform/eips/pull/41/files?diff=unified&w=1#r927416773

[coveralls]

Pull Request Test Coverage Report for Build 2838458722

• 404 of 626 (64.54%) changed or added relevant lines in 14 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+3.5%) to 65.129%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core/src/cli_commands.rs	0	1	0.0%
core/src/cli_commands/vote_update_pool.rs	0	1	0.0%
core/src/box_kind/update_box.rs	16	19	84.21%
core/src/box_kind/ballot_box.rs	60	71	84.51%
core/src/scans.rs	0	16	0.0%
core/src/main.rs	0	17	0.0%
core/src/cli_commands/bootstrap.rs	0	18	0.0%
core/src/serde.rs	2	20	10.0%
core/src/cli_commands/prepare_update.rs	161	185	87.03%
core/src/oracle_state.rs	0	50	0.0%

Files with Coverage Reduction	New Missed Lines	%
core/src/cli_commands.rs	1	0%

Totals
Change from base Build 2830258606:	3.5%
Covered Lines:	1963
Relevant Lines:	3014

---

💛 - Coveralls

[SethDusek]

Currently trying to get a simple update operation working on testnet, unfortunately it's being a bit spotty right now in terms of blocks.

Also, I was wondering what the interface should be like for updating. Currently I have a rather simple setup where update-pool reads oracle_config.yaml, and compares refresh NFT/update NFT tokens to the pool box's parameters and sets it up from that. (No support for new reward tokens currently in the interface). Ideally though we should have some sort of partial bootstrap feature, since right now I'm testing it with a refresh NFT I minted myself and that isn't guarded by any contract.

[greenhat]

Currently trying to get a simple update operation working on testnet, unfortunately it's being a bit spotty right now in terms of blocks.

Great!

Also, I was wondering what the interface should be like for updating. Currently I have a rather simple setup where update-pool reads oracle_config.yaml, and compares refresh NFT/update NFT tokens to the pool box's parameters and sets it up from that. (No support for new reward tokens currently in the interface). Ideally though we should have some sort of partial bootstrap feature, since right now I'm testing it with a refresh NFT I minted myself and that isn't guarded by any contract.

Oh yeah, the new refresh NFT should be in a box with the new refresh contract. Otherwise, oracles are voting for a blank check, right? Since we need to have a new refresh contract with a new refresh NFT before the voting, I suppose a new command is in order. Something like `UpdateRefreshContract`` which mints the new refresh NFT and puts it in the box with the new refresh contract.
@kettlebell What do you think?

[SethDusek]

Hmm, the method I had in mind was to rework/copy the bootstrap steps, but allow specifying tokens/contracts for each step in the process. So if for example, you provide a pool Box, it won't mint a new one. Then you could maybe have something like update_config.yaml, that has optional parameters for each contract, something like:

#[derive(Serialize, Deserialize)]
struct UpdateConfig {
    update_contract_parameters: Option<UpdateContractInputs>,
    refresh_contract_parameters: Option<RefreshContractInputs>, //and so on.
}

[greenhat]

That's interesting. I see how the update is similar to the bootstrap.
However, I'd refrain from adding complexity to the bootstrap code. We need to remember about the maintenance and the bar for new developers.
I'd prefer to have a separate code for contract updates sharing some generic/common code with bootstrap. I believe updating will be the most complex process we have. That's a good reason to compartmentalize it so that it does not leak into existing commands.

[kettlebell]

The UpdateRefreshContract command sounds good to me.

[SethDusek]

https://testnet.ergoplatform.com/en/transactions/abea0c5e6866bf8859b0ae004e864df0ef826fe4939569689fb5759436ca79d2

Successfully did a pool update on testnet with new refresh NFT, albeit I had to mint it myself for the time being

[kettlebell]

https://testnet.ergoplatform.com/en/transactions/abea0c5e6866bf8859b0ae004e864df0ef826fe4939569689fb5759436ca79d2

Successfully did a pool update on testnet with new refresh NFT, albeit I had to mint it myself for the time being

Nice!

[SethDusek]

During the above mentioned testnet try, I ran into a small issue regarding the Update Contract. I had some ballot tokens still in my wallet, and as we need an input box from wallet for fees, the update contract failed because the registers were not defined:

  def isValidBallot(b:Box) = if (b.tokens.size > 0) {
    b.tokens(0)._1 == ballotTokenId       && // Wallet box contained the token
    b.R5[Int].get == SELF.creationInfo._1 && // Called Option.get on None
    b.R6[Coll[Byte]].get == poolOutHash   &&
    b.R7[Coll[Byte]].get == rewardTokenId && 
    b.R8[Long].get == rewardAmt             
  } else false

I don't think however that we necessarily need to update the contract, in theory adding a check for b.R8[Any].isDefined before accessing the registers would work, but it'd increase the size of the contract. Second, eventually we'd have a way to transfer ballot tokens from oracle-core, so there wouldn't be any need to keep the ballot tokens in the wallet anymore. For now I guess I could add a warning/error if the wallet address contains ballot tokens

[greenhat]

Hi, here's some initial work for #49.

I have some questions.

1. Is there any case where a user might want to update the reward tokens used in the oracle, without making any other changes (new refresh NFT, etc)? Because right now, the update contract requires that the new pool's script be different from the old one, so simply replacing say old reward tokens with a new token, or adding more tokens to the pool is not possible.

This requirement is now removed from the contract. See ergoplatform/eips#41 (comment) . Please, update the contract on our side.

[greenhat]

During the above mentioned testnet try, I ran into a small issue regarding the Update Contract. I had some ballot tokens still in my wallet, and as we need an input box from wallet for fees, the update contract failed because the registers were not defined:

  def isValidBallot(b:Box) = if (b.tokens.size > 0) {
    b.tokens(0)._1 == ballotTokenId       && // Wallet box contained the token
    b.R5[Int].get == SELF.creationInfo._1 && // Called Option.get on None
    b.R6[Coll[Byte]].get == poolOutHash   &&
    b.R7[Coll[Byte]].get == rewardTokenId && 
    b.R8[Long].get == rewardAmt             
  } else false

I don't think however that we necessarily need to update the contract, in theory adding a check for b.R8[Any].isDefined before accessing the registers would work, but it'd increase the size of the contract. Second, eventually we'd have a way to transfer ballot tokens from oracle-core, so there wouldn't be any need to keep the ballot tokens in the wallet anymore. For now I guess I could add a warning/error if the wallet address contains ballot tokens

Nice find! I wonder if ballot tokens would be all sent out to the operators. I mean, I can imagine "bootstraper" can have some extra ballot tokens left in their wallet for future participants and whatnot. OTOH, update pool box operation most likely would be carried out after the pool had been working for quite some time. So it's not that unreasonable to require "no extra ballot tokens" for the operator that runs the update operation. I'd rather leave it as is with a warning. @kettlebell WDYT?

BTW, I did not plan any command to send out the ballot tokens. It can be done with a regular send command in the wallet.

[kettlebell]

Nice find! I wonder if ballot tokens would be all sent out to the operators. I mean, I can imagine "bootstraper" can have some extra ballot tokens left in their wallet for future participants and whatnot. OTOH, update pool box operation most likely would be carried out after the pool had been working for quite some time. So it's not that unreasonable to require "no extra ballot tokens" for the operator that runs the update operation. I'd rather leave it as is with a warning. @kettlebell WDYT?

BTW, I did not plan any command to send out the ballot tokens. It can be done with a regular send command in the wallet.

Yeah I agree that the bootstrapper could very well have extra ballot tokens especially if he wants to keep control of the updating process until things settle and the other participants are trustworthy enough to deserve a ballot token.

It's extra work on your part @SethDusek, but how about excluding any ballot-holding boxes from the wallet for the inputs? In theory this could mean no boxes would be available for fees, then we could return an error for the operator to remedy this. What do you guys think?

[greenhat]

It's extra work on your part @SethDusek, but how about excluding any ballot-holding boxes from the wallet for the inputs? In theory this could mean no boxes would be available for fees, then we could return an error for the operator to remedy this. What do you guys think?

I like this idea! I mean most likely there will be enough boxes for a fee without a box with ballot tokens. Otherwise, a fatal error is in order with a recommendation to move ballot tokens to another wallet.

[SethDusek]

Thank you for the detailed review, I'll work on the changes and re-request review when it's ready.

[greenhat]

Looking great! Thank you! Just a typo in doc comment.