-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(iota-genesis-builder): Add configurable delegator to genesis CLI #4346
base: develop
Are you sure you want to change the base?
Conversation
6043c59
to
7e90668
Compare
…token_distribution_schedule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nonast looks good in general. Made a few minor suggestions that you could consider.
…schedule for clarity
let mut timelock_objects = | ||
pick_objects_for_allocation(timelocks_pool, target_stake, &mut timelock_surplus); | ||
if !timelock_objects.to_burn.is_empty() { | ||
// Inside this block, the previous surplus is empty so we update it (possibly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a bit confusing to me, why is it empty here? Is it because pick_objects_for_allocation
enforces that? If yes, maybe we should add that to the comment. At first look I would say that the if condition checks that it's not empty, but then that's the to_burn
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, shouldn't we check that it is really empty before replacing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing that comes to my mind.... why do we even "burn" something? Is it because we consume those "UTXO's"? If yes, shouldn't we rather call it to_consume
instead of to_burn
?
Burning, at least in my head, is only used if we actually destroy tokens that are gone forever afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are technically "draining" the surplus object from its surplus_nanos. Therefore, the surplus object can be burned.
In the pick_objects_for_allocation
function, we try and take all the nanos that we need from the surplus object first. If we have "drained" the surplus object, we place it in the timelock_objects.to_burn
Vec. This means that the previous surplus is empty, and it needs to be replaced with a new one (picked in pick_objects_for_allocation
because the previous allocation might have needed some extra nanos).
Inside pick_objects_for_allocation this happens:
surplus = Coin(50)
- if target_stake == 49 then surplus = Coin(1); timelock_objects.amount_nanos = 49; timelock_objects.to_burn = empty
- if target_stake == 50 then surplus = empty; timelock_objects.amount_nanos = 50; timelock_objects.to_burn = [surplus]
- if target_stake == 51 then surplus = empty; timelock_objects.amount_nanos = 51; timelock_objects.to_burn = [surplus, timelock_1]
Btw, shouldn't we check that it is really empty before replacing it?
This is a given, since we're in the scope of timelock_objects.to_burn.is_empty()
. If the to_burn
field is empty, it means we still have nanos in the surplus object. Would you advice adding a check here out of safety/clarity? Or could some comments explaining this be sufficient?
Another thing that comes to my mind.... why do we even "burn" something? Is it because we consume those "UTXO's"? If yes, shouldn't we rather call it to_consume instead of to_burn?
Burning, at least in my head, is only used if we actually destroy tokens that are gone forever afterwards.
Indeed, at first glance I also think we could use another term for this, something more in line with what's being used throughout the code: to destroy
empty objects.
However, these objects are not really "consumed" in the actual object database yet. This happens in a later stage in the build_unsigned_genesis_data
phase of the ceremony. First, the required genesis objects are created. After this, the genesis objects are being "cleansed" from the to be burned objects that we have specified earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed burn into destroy here 1ef83d2
/// The surplus amount for that coin object. | ||
surplus_nanos: u64, | ||
/// Possibly indicate a timelock stake expiration. | ||
timestamp: u64, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to use an option here?
I mean someone could still create an UTXO with a timelock and set the timelock to zero.
Maybe that would lead to the same result, but I'm just wondering if using >0
logic is really the cleanest way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. I guess it makes sense to use an Option here if there should be a clear distinction between surplus being 0 and non-existent. In this case, I don't think it's necessary to convert this to an Option. It would add additional complexity to the code (checks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During the migration from UTXO to Object, here we check if the timelock still holds at the "target milestone timestamp". If it is not timelocked at that time then we treat is as a simple basic output. This process is performed before this genesis building step, so we can't have unwanted behaviors like your scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Just some small renaming and comment fixes
dea22ae
to
e052623
Compare
e052623
to
dea22ae
Compare
Description of change
To make the genesis delegator(s) configurable by the cli, we need to make changes to two iota commands:
iota genesis
iota genesis-ceremony
This draft-PR contains the progress of this task, in which the changes to both commands have been made to allow for a custom delegation configuration through the CLI. The
iota-genesis
command expects a single delegator when a migration snapshot has been provided. Theiota genesis-ceremony
now contains a new sub command calledinit-delegations
and expects a path to a.csv
file as argument, which contains the custom delegation configuration. The command is expected to be run before thebuild-unsigned-checkpoint
command.This csv file has the following format:
Links to any relevant issues
fixes #4218
Type of change
Choose a type of change, and delete any options that are not relevant.
How the change has been tested
Tested the local genesis:
cargo run --release --bin iota genesis --working-dir ./test_iota_config -f --with-faucet --num-validators 2 --remote-migration-snapshots https://stardust-objects.s3.eu-central-1.amazonaws.com/iota/alphanet/test/stardust_object_snapshot.bin.gz --delegator 0x4f72f788cdf4bb478cf9809e878e6163d5b351c82c11f1ea28750430752e7892
cargo run --release --bin iota start --network.config test_iota_config --with-faucet
Tested the following affected example:
cargo run --example snapshot_only_test_outputs --features="test-outputs" iota --snapshot-path ../latest-full_snapshot.bin --delegator 0x4f72f788cdf4bb478cf9809e878e6163d5b351c82c11f1ea28750430752e7892
Tested running the genesis ceremony, which resulted in a
genesis.blob
andmigration.blob
, which were then used to run a local network withiota start
. Using the explorer on this local network showed the correct delegations.todo: the
validate-state
command of the genesis ceremony is broken when used with a migrated state. This step was therefore skipped in the tests mentioned above.Change checklist
Tick the boxes that are relevant to your changes, and delete any items that are not.