Conversation
pmconrad
left a comment
pmconrad
left a comment
There was a problem hiding this comment.
General remarks:
- Please don't change the formatting. It makes reviews more difficult because a diff shows more changes than there actually are, and even though the current formatting is a bit unusual it is at least mostly consistent.
- Please read https://github.com/bitshares/bitshares-core/wiki/Git-Flow to understand the purpose of each branch. This particular change should be rebased onto the "release_candidate" branch, and that's where the PR should merge it.
- Please make sure the code compiles before pushing it, and make sure the unit tests are working before creating a PR.
| { | ||
| const asset amount_to_issue = o.amount * fhistory.effective_median_history; | ||
| const asset amount_to_subtract = o; | ||
| amount_to_issue.amount.asset_id = MBD_SYMBOL; //making sure it's the new MBD asset id for safety |
There was a problem hiding this comment.
Does this even compile? I think the amount. is too much. Same in following lines.
| p.virtual_supply += amount_to_issue * fhistory.effective_median_history; | ||
| } ); | ||
| } | ||
| else if( o.amount.asset_id == BTCM_SYMBOL ) //this operation will convert MUSE into BTCM |
There was a problem hiding this comment.
This is a possible way to do it. However,
- it is confusing because now for some assets you have to specify the amount you want to have converted, and for some you have to specify the amount to receive from the conversion
- you cannot also add conversion of BTCM->MUSE nor BTCM->VESTS
| if( o.amount.asset_id == MUSE_SYMBOL ) | ||
| { | ||
| const asset amount_to_issue = o.amount * fhistory.effective_median_history; | ||
| const asset amount_to_subtract = o; |
There was a problem hiding this comment.
You cannot assign o here. Did you mean 0?
| } | ||
| else if( o.amount.asset_id == BTCM_SYMBOL ) //this operation will convert MUSE into BTCM | ||
| { | ||
| const asset amount_to_issue = o; |
| muse::chain::asset_id_type VESTS_SYMBOL=(muse::chain::asset_id_type(1)); | ||
| muse::chain::asset_id_type MBD_SYMBOL=(muse::chain::asset_id_type(2)); | ||
| muse::chain::asset_id_type MBD_SYMBOL=(muse::chain::asset_id_type(13)); //changing MBD id from 2 to 13 |
There was a problem hiding this comment.
Don't do that. This will retroactively change MBD_SYMBOL, which means that you cannot replay the existing blockchain. What you can do is add a new XUSD_SYMBOL, and change the code to use XUSD_SYMBOL or MBD_SYMBOL depending on whether it's before or after the consensus upgrade.
Also, you shouldn't use fixed IDs here. The 3 base assets are created by the core code automatically and therefore have known IDs, but anything after these will be assigned an ID upon creation. One way to deal with this could be to register the assets manually both in testnet and in mainnet, and then insert the resulting IDs into the codebase (which will likely be different between mainnet and testnet).
| #define MUSE_MAX_MEMO_SIZE 2048 | ||
| #define MUSE_MAX_PROXY_RECURSION_DEPTH 4 | ||
| #define MUSE_VESTING_WITHDRAW_INTERVALS 13 | ||
| #define MUSE_VESTING_WITHDRAW_INTERVALS 4 //changing vesting period from 13 weeks to 4 weeks |
There was a problem hiding this comment.
See above: you cannot simply change this because the change would happen retroactively. You can define a new interval, and use one or the other depending on whether it happens before or after the upgrade.
2 participants
No description provided.