Skip to content
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

[#2130] adm: Add dump-nns and restore-nns commands #2138

Conversation

KirillovDenis
Copy link
Contributor

close #2130

@codecov
Copy link

codecov bot commented Dec 7, 2022

Codecov Report

Attention: Patch coverage is 3.50195% with 248 lines in your changes missing coverage. Please review.

Project coverage is 30.53%. Comparing base (42554a9) to head (b28c586).
Report is 1410 commits behind head on master.

Files with missing lines Patch % Lines
cmd/neofs-adm/internal/modules/morph/nns.go 0.00% 241 Missing ⚠️
cmd/neofs-adm/internal/modules/morph/root.go 56.25% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2138      +/-   ##
==========================================
- Coverage   30.78%   30.53%   -0.25%     
==========================================
  Files         382      383       +1     
  Lines       28056    28313     +257     
==========================================
+ Hits         8637     8646       +9     
- Misses      18687    18935     +248     
  Partials      732      732              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@KirillovDenis
Copy link
Contributor Author

Currently, this PR sub-optimal in terms of tx sending (we await txs for every domains, this should be fixed a little bit later).

Also there are the following problems/questions:

  • NNS contract doesn't have methods to get owner and admin for specific domain. Should we try to get this info using raw contract storage (it seems that some owners should be changed after redeployment (e.g. for container domain))?
  • If domain already has some records should we 1) add missed records, 2) overwrite all records 3) skip restoring this domain ?

/cc @alexvanin @fyrchik

@carpawell
Copy link
Member

carpawell commented Oct 17, 2024

@roman-khimov, @cthulhu-rider, I have checked this PR, TBH, I would redo some things and also it is from an archive repo now. Do not understand the 1st question, we have (and seems like always had OwnerOf method). The second one is a regular question, we just need a consensus here (I think "add missed records" is the way).

Suggest closing this PR, or merging it and continuing immediately with extensions via another PRs. An opened draft for two+ years makes me nervous.

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This won't merge without update
  2. Some things need to be fixed, but they will take some time
  3. I don't believe in dump/restore concept, it's very suspicious to me and I'm not sure we can easily survive this.
  4. I've never seen this being done in practice and never needed it.
  5. We'll have Using NeoFS to store blocks and snapshots neo-project/neo#3463 eventually, so chain migration doesn't seem to be valuable at all.
  6. If needed, the code won't go away and we can return to it.

if isAvailable {
soa := domain.SOA
emit.AppCall(bw.BinWriter, ch, "register", callflag.All,
soa.Name, wCtx.ConsensusAcc.Contract.ScriptHash(), soa.Email, soa.Refresh, soa.Retry, soa.Expire, soa.TTL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's broken here. Domains have owners and we're not keeping them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

neofs-adm: add export/import NNS domains
3 participants