FM-316: Top down mint increase circ supply #330
Conversation
aakoshh
changed the title
| params: FvmUpdatableParams, | ||
|
|
||
| /// Indicate whether the parameters have been updated. | ||
| params_dirty: bool, |
There was a problem hiding this comment.
Just curious, I see this parameter updated but not read, is it used in other PRs?
There was a problem hiding this comment.
You are right, it's a bit obscure. The only reason I added it was to use it in genesis where the commit, like the one here used to, returns just a CID but it's a unified one coming either from the StateTree or the FvmExecState. I didn't want to change that commit too, but I wanted to have a sanity check that there hasn't been some change that I am discarding, in which case this design would have to change too, e.g. by moving the fields from FvmGenesisOutput to the commit return value.
It's a bit convoluted, but I haven't found a nice way to do things. I tried with maintaining a copy of FvmStateParams, but not all those fields can change during a run, and for the ones that can be retrieved from the executor, there is always that ambiguity of which one to return. Now there is only one, because the circ_supply in the updatable part is write-only.
This is tracked by the parent every time funds move down, but the FVM also needs to track of its circulating supply. With this we have an accurate view of what is the circulating supply of a subnet. Even more, this is the most accurate value for the supply. Even if the parent says that the circulating supply of the subnet is 10FIL, that is just what was frozen to be injected in the subnet, but the real circulating supply in the subnet may be less because some top-down messages haven't been executed yet, or because funds have been burnt. Even more, this opens the door to something we have discussed for a while, the ability to reward with native tokens in the subnet, producing some inflation in the subnet (producing a dynamic exchange rate between the child subnet and the parent's token). This is completely out of scope in the short-term, but worth noting. |
|
So the circulating supply will be tracked in both the parent and child. It is possible that these two values will be out of sync and eventually be consistent? |
| /// | ||
| /// This is just a technical thing to help us not forget about saving something. | ||
| /// | ||
| /// TODO: `base_fee` should surely be here. |
There was a problem hiding this comment.
Nice one, I like this approach! Yes, I was actually going to ask you why the base_fee wasn't included here.
I would include it for now even if it doesn't currently change as we may want to expose a callback to update it for users in the future. But maybe you have a reason not to include it yet, so happy either way.
There was a problem hiding this comment.
I didn't include it purely because there is no user story for how to update it. I created an issue, but haven't checked what's happening in Lotus or Forest. Maybe it's based on the last N blocks or something? Or maybe it's based on some voting, I don't know. I'd rather have a TODO stating it's not done, then code which suggests it might be done, which sends people looking all over to code just to confirm that nothing is happening.
But the history of it is already maintained, so as soon as we add it here, the compiler will tell us to update the value in app.rs and all will be well.
There was a problem hiding this comment.
I would keep a static base_fee and not use what Lotus is doing, I would rather have a way for users to decide the base_fee function instead of giving them a really complicated one that they don't understand. So yes, let's leave it for the future. Thanks!
|
|
||
| state.update_circ_supply(|circ_supply| { | ||
| *circ_supply += total_value; | ||
| }); | ||
|
|
||
| gateway_caller.apply_cross_messages(state, messages) |
There was a problem hiding this comment.
I think we already discussed this when we were reviewing top-down, and we said we would defer it to the future, but what is your take for when executing the message fails?
There was a problem hiding this comment.
I had to re-read my comments:
To reiterate the main points:
- Individual messages in the batch should be allowed to fail without failing the whole batch
- Failing the whole batch and moving on is unacceptable, especially if we can't roll back the validator changes and the parent height finality
- But, the whole match might fail if it goes over the block gas limit
- Therefore we should:
- do all 3 parts as an atomic call to Solidity
- let the whole thing fail as a transaction and be retried by another relayer
- not execute immediately (and hit the block gas limit) but enqueue for execution and do it when we have room
- then we can treat individual messages with their own gas limit
Generally the should end up being consistent, but there may be cases where they are not (like funds being burnt directly in the child without being released to the parent).
|
The reason for tracking it in Fendermint is that the circulating supply is basically a constructor parameter for an FVM instance, so at the very least it has to be accessible to Fendermint. Technically it could live in the state of an actor, like the |
If you send tokens to a burner address, the host will have no knowledge of this, and the circulating supply will not reflect the permanent locking of them. There is a burntfunds actor which we could use to adjust the circulating supply down, but people aren't obliged to use it. |
Co-authored-by: cryptoAtwill <[email protected]>
Closes consensus-shipyard/ipc#270
The PR changes the top-down message execution so that it increases the circulating supply with the tokens minted and given to the gateway.