-
Notifications
You must be signed in to change notification settings - Fork 50
Feat/meta data native contract #3759
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
base: master
Are you sure you want to change the base?
Conversation
cthulhu-rider
left a comment
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.
rebase is needed
| panic(fmt.Sprintf("unexpected native contract found: %T", contract)) | ||
| } | ||
| } | ||
| return append(newContracts, meta.NewMetadata(neoContract)) |
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.
meta.NewMetadata does not seem resistant to nil arg. I'd precheck it
in general, looking at for-loop, we dont check absence of any contract. If they are required, i'd still prevent this
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.
added required contracts checks to NewCustomNatives
| newContracts = append(newContracts, contract) | ||
| case *native.Std, *native.Crypto, *native.Oracle, *native.Treasury: | ||
| default: | ||
| panic(fmt.Sprintf("unexpected native contract found: %T", contract)) |
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.
is this done to not miss new contract?
since native.NewDefaultContracts does not state there will be no more contract, i'd say this is an error
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.
A new contract is a significant (and very rare) event, this forces explicit handling of it. Should be ok.
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.
is this done to not miss new contract?
yes, *native.Treasury was even added after this feature was started to be implemented. my idea was that once there is an update in neo-go, tests will start panicking, and we will immediately find out whether we need to adopt a new contract to our side chain or not
| g := &GAS{ | ||
| initialSupply: init, | ||
| } | ||
| defer g.BuildHFSpecificMD(g.ActiveIn()) |
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.
defer seems redundant
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.
this is the way every native contract is implemented in neo-go, so i suggest to keep it
| return nil | ||
| } | ||
|
|
||
| // InitializeCache implements the Contract interface. |
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.
| // InitializeCache implements the Contract interface. | |
| // InitializeCache implements [native.IGAS] interface. |
for all methods
| } | ||
|
|
||
| // makeUint160Key creates a key from the account script hash. | ||
| func makeUint160Key(prefix byte, h util.Uint160) []byte { |
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.
unused too? Seems like some stuff is copy-pasted but not needed
| return fmt.Errorf("%d vector has incorrect REP: %w", i, err) | ||
| } | ||
| rep := repB.Int64() | ||
| if rep > maxREPsClauses { |
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.
i'd also check negatives
| Nodes keys.PublicKeys | ||
| } | ||
|
|
||
| func (p Placement) ToStackItem() (stackitem.Item, error) { |
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.
used anywhere?
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.
for some reason lost resolution for this thread: #3742 (comment)
is used now
| } | ||
| metaInfo, ok := metaInfoSI.Value().([]stackitem.MapElement) | ||
| if !ok { | ||
| panic(fmt.Errorf("unexpected deserialized meta information value: expected %T, %T, given", metaInfo, metaInfoSI.Value())) |
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.
| panic(fmt.Errorf("unexpected deserialized meta information value: expected %T, %T, given", metaInfo, metaInfoSI.Value())) | |
| panic(fmt.Errorf("unexpected deserialized meta information value: expected %T, %T given", metaInfo, metaInfoSI.Value())) |
| } | ||
|
|
||
| if foundSigs == rep { | ||
| continue nodesLoop |
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.
instruction seems excessive here, condition may be inverted
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.
reimplementation of the byte-contract code from the neofs-contracts problems. fixed
| panic("incorrect vub") | ||
| } | ||
| if vub.Int64() <= int64(ic.BlockHeight()) { | ||
| panic("incorrect vub: exceeded") |
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.
runtime values may be helpful in exception
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.
added values everywhere a value is parsed and can be stringified
| @@ -0,0 +1,45 @@ | |||
| package contracts | |||
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.
I'd strip this directory, can be metachain.
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.
dont mind
| newContracts = append(newContracts, contract) | ||
| case *native.Std, *native.Crypto, *native.Oracle, *native.Treasury: | ||
| default: | ||
| panic(fmt.Sprintf("unexpected native contract found: %T", contract)) |
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.
A new contract is a significant (and very rare) event, this forces explicit handling of it. Should be ok.
| ) | ||
|
|
||
| // nep17TokenNative represents a NEP-17 token contract. | ||
| type nep17TokenNative struct { |
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.
Wow. This should be imported from NeoGo native, really.
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.
really sorry, but it is not now
| m.AddMethod(md, desc) | ||
|
|
||
| desc = native.NewDescriptor("registerMetaContainer", smartcontract.VoidType, | ||
| manifest.NewParameter("cID", smartcontract.Hash256Type)) |
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.
container
| m.AddMethod(md, desc) | ||
|
|
||
| desc = native.NewDescriptor("unregisterMetaContainer", smartcontract.VoidType, | ||
| manifest.NewParameter("cID", smartcontract.Hash256Type)) |
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.
Here as well.
| panic("container does not support chained metadata") | ||
| } | ||
|
|
||
| sigsVectorsRaw, ok := args[1].Value().([]stackitem.Item) |
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.
This follows current container contract, but eventually it can be removed from parameters, nodes can sign transaction directly.
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.
this will require responding there with a signature not of an object's metadata, but with a signature of some predefined transaction. is that what you meant?
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.
Yes. Another option is a custom verification callback.
| } | ||
| } | ||
|
|
||
| // required |
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.
Why cid is checked separately?
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.
it is used in other checks above (container must exist and be registered as one that supports meta)
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.
I'd rather have similar checks at the same place.
|
|
||
| for _, sig := range sigVectors[i] { | ||
| for _, node := range placement[i].Nodes { | ||
| if node.Verify(sig, metaHash) { |
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.
Slow.
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.
i only suggest sorting signatures by public keys. requires support on the IR side and on the SN side
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.
It's not just about sorting. Remember, currently signatures are verified in verify() method of Proxy contract. This scales well. Checking them during transaction execution doesn't.
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.
But sorting is beneficial anyway and easily done.
| } | ||
| if v, ok := getFromMap(metaInfo, "firstPart"); ok { | ||
| firstPart, err := v.TryBytes() | ||
| if err != nil || len(firstPart) != smartcontract.Hash256Len { |
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.
sha256 extraction is pretty popular
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.
ok-ok
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
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 it, no processing?
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.
do you mean the secondary index must be in the same chain? my first implementation only needs side chain to check metadata and then notify about it. all the storage nodes handle notification on their own
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.
Among things current structure tries to solve is state synchronization. There is everything needed for it in blockchain, but if you're just throwing an event it all doesn't work. Primary index (based on submitted data) must be built directly on-chain. Then seconday one (header-based) is a different story.
bd68082 to
c8ae0ca
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3759 +/- ##
==========================================
- Coverage 25.77% 25.74% -0.03%
==========================================
Files 657 659 +2
Lines 42163 42195 +32
==========================================
- Hits 10867 10863 -4
- Misses 30308 30344 +36
Partials 988 988 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Also, implement new MetaData contract and GAS contract implementation without economic. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
c8ae0ca to
1f840ee
Compare
Contract part of #3742.