Skip to content
This repository has been archived by the owner on Oct 4, 2019. It is now read-only.

Operations sequence #259

Closed
asuleymanov opened this issue Nov 10, 2017 · 21 comments
Closed

Operations sequence #259

asuleymanov opened this issue Nov 10, 2017 · 21 comments
Assignees
Labels

Comments

@asuleymanov
Copy link

asuleymanov commented Nov 10, 2017

Приветствую.
Из-за изменения последовательности операций в массиве. Большинство сторонних библиотек на других ЯП при переходе на 17 версию не смогут работать. Для этого программистам библиотек необходимо будет переписывать их. А можно попросить сделать более правильно (на мой взгляд).
Т.е. что я имею ввиду новые операции которые будут в 17 версии переместить в начало массива, а операции оставленные для 16 версии переместить в конец массива. Ведь сами операции и входные параметры не изменились. Но из-за сдвигания массива на 40 пунктов библиотеки и соответственно По ранее на них написанное перестанет работать. Вот как массив выглядит сейчас:

// _16 for 0.16.4
  vote_16: 0,
  comment_16: 1,
  transfer_16: 2,
  transfer_to_vesting_16: 3,
  withdraw_vesting_16: 4,
  limit_order_create_16: 5,
  limit_order_cancel_16: 6,
  feed_publish_16: 7,
  convert_16: 8,
  account_create_16: 9,
  account_update_16: 10,
  witness_update_16: 11,
  account_witness_vote_16: 12,
  account_witness_proxy_16: 13,
  pow_16: 14,
  //
  custom: 15,
  //
  report_over_production_16: 16,
  delete_comment_16: 17,
  //
  custom_json: 18,
  //
  comment_options_16: 19,
  set_withdraw_vesting_route_16: 20,
  limit_order_create2_16: 21,
  challenge_authority_16: 22,
  prove_authority_16: 23,
  request_account_recovery_16: 24,
  recover_account_16: 25,
  change_recovery_account_16: 26,
  escrow_transfer_16: 27,
  escrow_dispute_16: 28,
  escrow_release_16: 29,
  pow2_16: 30,
  escrow_approve_16: 31,
  transfer_to_savings_16: 32,
  transfer_from_savings_16: 33,
  cancel_transfer_from_savings_16: 34,
  //
  custom_binary: 35,
  //
  decline_voting_rights_16: 36,
  reset_account_16: 37,
  set_reset_account_16: 38,
  comment_benefactor_reward_16: 39,
  //
  vote: 40,
  comment: 41,
  transfer: 42,
  transfer_to_vesting: 43,
  withdraw_vesting: 44,
  limit_order_create: 45,
  limit_order_cancel: 46,
  feed_publish: 47,
  convert: 48,
  account_create: 49,
  account_update: 50,
  witness_update: 51,
  account_witness_vote: 52,
  account_witness_proxy: 53,
  pow: 54,
  report_over_production: 55,
  delete_comment: 56,
  comment_options: 57,
  set_withdraw_vesting_route: 58,
  limit_order_create2: 59,
  challenge_authority: 60,
  prove_authority: 61,
  request_account_recovery: 62,
  recover_account: 63,
  change_recovery_account: 64,
  escrow_transfer: 65,
  escrow_dispute: 66,
  escrow_release: 67,
  pow2: 68,
  escrow_approve: 69,
  transfer_to_savings: 70,
  transfer_from_savings: 71,
  cancel_transfer_from_savings: 72,
  decline_voting_rights: 73,
  reset_account: 74,
  set_reset_account: 75,
  comment_benefactor_reward: 76,
  delegate_vesting_shares: 77,
  account_create_with_delegation: 78,
  comment_payout_extension: 79,
  asset_create: 80,
  asset_update: 81,
  asset_update_bitasset: 82,
  asset_update_feed_producers: 83,
  asset_issue: 84,
  asset_reserve: 85,
  asset_fund_fee_pool: 86,
  asset_settle: 87,
  asset_force_settle: 88,
  asset_global_settle: 89,
  asset_publish_feed: 90,
  asset_claim_fees: 91,
  call_order_update: 92,
  account_whitelist: 93,
  override_transfer: 94,
  proposal_create: 95,
  proposal_update: 96,
  proposal_delete: 97,
  bid_collateral: 98,
@nemothenoone nemothenoone self-assigned this Nov 10, 2017
@bitphage
Copy link

Yeah this change breaks compatibility with all libraries. Tested with python golos libs.

@nemothenoone
Copy link
Member

Operations order is a backward compatibility requirement. All the operations signed before hardfork17 in some cases are getting identified by a number, not a name. So the order is a crucial stuff. This restriction is going to be removed in a softfork version following.

golos-js is already getting updated, golos-pyhton will be updated until the hardfork comes.

@asuleymanov
Copy link
Author

Я не говорю про библиотеки на ЯП js и python. Или другие языки программирования Вас мало интересуют?
Т.е. Вам как я понимаю все равно что у сторонних разработчиков приложения работать не будут?

@nemothenoone
Copy link
Member

nemothenoone commented Nov 13, 2017

@asuleymanov We have started notifying all the community API libraries developers. That is why we have some time after public testing beginning - to help community developers introduce protocol versioning for API libraries.

@asuleymanov
Copy link
Author

Так ладно понятно.
Итого получается что это так и останется. Ладно потерпим не беда.
Вы упоминали что это будет изменено после softfork. Как я понимаю это приведет к тому что все вернется обратно после перехода на ХФ? Если так то в какие сроки?

И уж если говорите что оповещали то можно посмотреть где это было сделано?

@nemothenoone
Copy link
Member

nemothenoone commented Nov 13, 2017

@asuleymanov Protocol versioning is not going to be reverted because of protocol structure changes mechanism existence requirements (long asset names or more votable chain params for example). It will be widely used in future. But don't be scared. There are no any backward compatibility obligations for API libraries. It just needs to be changed once and done.

We've notified some of community library developers in personal, but not in public, because of a reference API libraries have not been fully updated yet and we did not finished updating our wiki and doxygen-generated documentation. You were in notification queue after Steepshot guys :) In case you asked in person, I think, the time has come.

Just to solve the trouble for now. Changes are all about asset precision and ticker name serialisation. Version 16 had asset serialisation like 1000 | \x03 | GOLOS <=> amount | precision_decimals | ticker_name <=> 8 bytes | 1 byte | 6 bytes. Now it is 1000 | GOLOS | \x03 <=> amount | ticker_name | decimals <=> 8 bytes | 16 bytes | 1 byte.

So all the operations contained asset, price, price_feed, chain_properties structures needs to be updated.

I think @b1acksun could help you deal with it.

@asuleymanov
Copy link
Author

Спасибо большое за разъяснения.

@nemothenoone
Copy link
Member

nemothenoone commented Nov 13, 2017 via email

@asuleymanov asuleymanov reopened this Nov 13, 2017
@asuleymanov
Copy link
Author

asuleymanov commented Nov 13, 2017

Т.е. еще разок для закрепления у меня материала.
Массив операций такой и будет, и если и будет изменяться то только добавляя новые операции?

За разъяснения с значением amount огромное спасибо. Так как этот вопрос стоял в списке, но до него я просто не дошел. Как только реализую тестовую вариацию массива обязательно переделаю и проверю работоспособность. Обязательно по результатам отпишусь тут.

@asuleymanov
Copy link
Author

asuleymanov commented Nov 14, 2017

Приветствую.
Я переделал библиотеку под 17 версию и большинство функций работает нормально.

Предложенный @nemo1369 вариант был опробован и работает на отлично. Ну по крайней мере со стандартными токенами проблем не возникло. Буду тестировать и разрабатывать дальше.
Кому интересно код можно посмотреть тут

И да @nemo1369 большая просьба посмотреть issues/272.

@nemothenoone
Copy link
Member

nemothenoone commented Nov 14, 2017

Thanks for the report @asuleymanov.

Talking about feature changes. Operations variant is only going to be extended until got replaced. The only important changes for API libraries developers will be described in doxygen-generated documentation.

@t3ran13
Copy link

t3ran13 commented Nov 30, 2017

я чет не понял, у нас порядок операций изменится?

@nemothenoone
Copy link
Member

@t3ran13 Operations set was extended for versioned operations with fundamental structure changes.

@t3ran13
Copy link

t3ran13 commented Nov 30, 2017

но почему, например, операция vote теперь имеет порядок 40 вместо изначального 0?
это приведет к тому, что все старые подписи операций станут недействительными.

@nemothenoone nemothenoone changed the title Последовательность операций. Operations sequence Nov 30, 2017
@nemothenoone nemothenoone added this to the 0.2.0 milestone Nov 30, 2017
@nemothenoone
Copy link
Member

@t3ran13 Of course it is not, because stuff I've designed is about versioned operations. It means, daemon will be backward compatible with operations for versions 0-16, but it will not accept operations for version 17 just after hardfork time has come. Daemon with production network data is deployed at 46.101.132.158:2001 and 46.101.132.158:8090.

@t3ran13
Copy link

t3ran13 commented Dec 1, 2017

Есть патерн ивент сорсинг, который запрещает такое делать. Может появиться vote_2.0 ,например, но старый останется как есть.

Зачем , в данном случае, наращивать энтропию в архитектуре приложения?

Как вы прикажете действовать тем кто будет сканировать блоки напрямую? До блока Х делайте так, после блока Х делацте сяк? При этом имя операции остается одним и тем же

У меня мозг отказывается понимать архитектуру такого приложения.

Я не понимаю почему нельзя это обойти по человечески? Зачем создавать костыль и инструменты для работы с кастылем? Почему нельзя ввести операции версии 2.0?

@nemothenoone
Copy link
Member

nemothenoone commented Dec 1, 2017

@t3ran13 It was implemented exactly as you suppose. Internal implementation is like account_create_operation<0, 16, 0> for the versions 0-16 and account_create_operation<0, 17, 0> for the version 17, so operations proceeded are immutable. It satisfies the "Event Sourcing" pattern perfectly well (in fact it barely satisfies it).

Talking about JSON-RPC protocol - implementing account_create_operation_2 or account_create_operation_228 for each new protocol version makes it huge and messy. Actually, it was a tradeoff in favor of frontend developers to make the current communication protocol as tiny as we can. Full blockchain analysis would really require to implement multiple protocol versions support - just like golos-js does.

I think I just need to clean up the doxygen documentation to include the whole protocol description.

@t3ran13
Copy link

t3ran13 commented Dec 9, 2017

т.е. данные в самой цепочке будут храниться корректно? а нечеловеческий вид лишь для rpc? при этом это вызано требованием сократить на 2 символа имя и сэкономить трафик? а если через пол года еще операции - добавяться/изменяться, то у нас получится еще одна полная фигня?

не, давайте по человечески все делать, и чб в rpc было согласно "Event Sourcing" - vote & vote_17 (или vote_2.0), но текущая реализация... как потом разрабом с этим апи работать? ты лично будешь каждому рассказывать как все реализовано и работает?

@ALL кто-то может прочекать код чб блоки не пострадали?

@t3ran13
Copy link

t3ran13 commented Dec 10, 2017

детали все тут
https://golos.io/ru--khardfork/@t3ran13/vazhno-vozmozhnye-kosyaki-v-dannykh-cepochki-iz-za-khf-vazhno-mnenie-razrabov#@nemo1369/re-t3ran13-re-nemo1369-re-t3ran13-vazhno-vozmozhnye-kosyaki-v-dannykh-cepochki-iz-za-khf-vazhno-mnenie-razrabov-20171209t194255420z

в общем - изменения для апи только, но для апи это хоть и криво, но не критично)

@bitphage
Copy link

Как вариант, можно сделать версионирование апи. Т.е. в каждом запросе/ответе слать атрибут вида "api_version: 1". Имена операций на клиенте будут одинаковыми, но для разных версий API соответственно будет меняться соответствие имя-номер.

@t3ran13
Copy link

t3ran13 commented Dec 10, 2017

это тоже немного усложняет обработку, немного проще когда каждое поле имеет свою версию, если ответы оборачивать в объекты, это не проблема) хотя и ваш вариант по своему хорош. просто что делать когда ответ смешанный? тогда он не годится)

я просто понял, что сделано "как сделано" и никто переделывать сейчас желанием не горит

maslenitsa93 added a commit that referenced this issue Sep 3, 2018
maslenitsa93 added a commit that referenced this issue Sep 3, 2018
maslenitsa93 added a commit that referenced this issue Sep 3, 2018
maslenitsa93 added a commit that referenced this issue Sep 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants