Conversation
| panic(fmt.Sprintf("unexpected native contract found: %T", contract)) | ||
| } | ||
| } | ||
| return append(newContracts, meta.NewMetadata(neoContract)) |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
defer seems redundant
There was a problem hiding this comment.
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.
| // 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.
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.
i'd also check negatives
| Nodes keys.PublicKeys | ||
| } | ||
|
|
||
| func (p Placement) ToStackItem() (stackitem.Item, error) { |
There was a problem hiding this comment.
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.
| 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.
instruction seems excessive here, condition may be inverted
There was a problem hiding this comment.
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.
runtime values may be helpful in exception
There was a problem hiding this comment.
added values everywhere a value is parsed and can be stringified
| 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.
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.
Wow. This should be imported from NeoGo native, really.
There was a problem hiding this comment.
really sorry, but it is not now
There was a problem hiding this comment.
This should be imported from NeoGo native
I don't think so. NeoGo's nep17TokenNative contains a lot of code that is specific to native Neo/Gas implementation and that is not needed for the simple meta GAS contract. I'd say that meta GAS doesn't even need nep17TokenNative, just define all methods on the GAS itself.
| m.AddMethod(md, desc) | ||
|
|
||
| desc = native.NewDescriptor("registerMetaContainer", smartcontract.VoidType, | ||
| manifest.NewParameter("cID", smartcontract.Hash256Type)) |
| m.AddMethod(md, desc) | ||
|
|
||
| desc = native.NewDescriptor("unregisterMetaContainer", smartcontract.VoidType, | ||
| manifest.NewParameter("cID", smartcontract.Hash256Type)) |
| panic("container does not support chained metadata") | ||
| } | ||
|
|
||
| sigsVectorsRaw, ok := args[1].Value().([]stackitem.Item) |
There was a problem hiding this comment.
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.
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.
Yes. Another option is a custom verification callback.
| } | ||
| } | ||
|
|
||
| // required |
There was a problem hiding this comment.
Why cid is checked separately?
There was a problem hiding this comment.
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.
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.
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.
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.
But sorting is beneficial anyway and easily done.
There was a problem hiding this comment.
@roman-khimov, but what if number of nodes is bigger than 16? i found it occasionally when increased number of nodes in tests
do you mean that tx should be signed by nodes? or do you mean that we need Verify() method of the new native contract that should still get signatures from args and verify them (and, i guess, that new transactions should require the contract's signature to call Verify)?
There was a problem hiding this comment.
Multiple signatures for a single signer.
There was a problem hiding this comment.
@roman-khimov, did some multisignature magic, check, please, if i got you right
| } | ||
| 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.
sha256 extraction is pretty popular
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
@roman-khimov, ok, adopted the previous metaservice approach. BUT currently there is oID+cID==64 problem for storage keys. i think we will eventually find a "one chain per container" solution, and it will be easier to work with keys, but currently i can only suggest skipping a single byte from oID
There was a problem hiding this comment.
Why do you care much about key size limits in native code?
bd68082 to
c8ae0ca
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3759 +/- ##
==========================================
- Coverage 25.73% 25.70% -0.03%
==========================================
Files 670 672 +2
Lines 43062 43091 +29
==========================================
- Hits 11081 11078 -3
- Misses 30969 30998 +29
- Partials 1012 1015 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c8ae0ca to
1f840ee
Compare
b7a798d to
13c1f32
Compare
978b2b0 to
4844514
Compare
roman-khimov
left a comment
There was a problem hiding this comment.
Better, but still can be optimized.
| g := &GAS{ | ||
| initialSupply: init, | ||
| } | ||
| defer g.BuildHFSpecificMD(g.ActiveIn()) |
| ) | ||
|
|
||
| // nep17TokenNative represents a NEP-17 token contract. | ||
| type nep17TokenNative struct { |
There was a problem hiding this comment.
This should be imported from NeoGo native
I don't think so. NeoGo's nep17TokenNative contains a lot of code that is specific to native Neo/Gas implementation and that is not needed for the simple meta GAS contract. I'd say that meta GAS doesn't even need nep17TokenNative, just define all methods on the GAS itself.
| metaInfoSI, err := stackitem.Deserialize(metaInfoRaw) | ||
| if err != nil { | ||
| panic(fmt.Errorf("cannot deserialize meta information from byte array: %w", err)) | ||
| } | ||
| 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.
Is there any usual contract that interacts with serialized metaInfo map? If not, then we may change the serialization format to bytes or raw JSON (like it's done for native ContractManagement's NEF/Manifest arguments of deploy/update), no need to keep stackitem.Map.
There was a problem hiding this comment.
Native Neo format has lower overhead compared to JSON and I don't see any immediate benefits of JSON here (for manifests that's rather easy, there are a lot of tools that are interested in manifests).
|
|
||
| // NewCustomNatives returns custom list of native contracts for metadata | ||
| // side chain. Returned contracts: | ||
| // - NEO |
There was a problem hiding this comment.
@roman-khimov, we don't need default NeoContract for this chain. We don't need voting and rewards at all, is it true?
There was a problem hiding this comment.
i think, these lines require me to use smth, i tried to drop NEO contract. changing NEO to smth no-op can be done, i went the easiest way
There was a problem hiding this comment.
do you mean to fill it with smth different just to cut out voting and all?
There was a problem hiding this comment.
You need to implement custom Neo contract (like you did it with custom Gas). This contract should implement minimal required functionality for meta chain: committee tracking and committee witness checking, that's it (although I'm not sure whether we need dynamic committee at meta chain; let's start with static set of keys). The rest of INEO functionality should be stubbed. Voting should be dropped.
There was a problem hiding this comment.
You need to implement custom Neo contract
why i need it?
There was a problem hiding this comment.
Currently, we have decided to use regular Neo native since its excess functionality does not bother us for now, in the future, we may add a new minimal Neo contract
Required for nspcc-dev/neofs-node#3759. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Required for nspcc-dev/neofs-node#3759. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Required for initial version of Meta native contracts constructor, ref. nspcc-dev/neofs-node#3759. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
00d5d10 to
6eee48f
Compare
|
Pushed smth wrong... |
6eee48f to
be48655
Compare
be48655 to
35c50ed
Compare
|
Fixed. |
roman-khimov
left a comment
There was a problem hiding this comment.
Specific indexes can be tuned, but signatures seem to be ok. Let's test it.
| md = native.NewMethodAndPrice(m.updateContainerList, 1<<15, callflag.WriteStates) | ||
| m.AddMethod(md, desc) | ||
|
|
||
| eDesc := native.NewEventDescriptor(putObjectEvent, |
| } | ||
|
|
||
| key[0] = addrIndex | ||
| deleteStorageItem(ic, key) |
There was a problem hiding this comment.
hm, tbh, i do not know whether it should be double checked on the chain side. if every SN agrees that this object should be PUT, should a chain reject it?
|
|
||
| for i, vector := range placement { | ||
| var foundSigs, lastFoundSig int | ||
| for _, sig := range sigVectors[i] { |
There was a problem hiding this comment.
There is an optimized multisig checker in NeoGo, but likely not easily applicable.
5e26b1d to
7648f14
Compare
29d72e2 to
2ecbf8a
Compare
2ecbf8a to
36316ab
Compare
Also, implement new MetaData contract and GAS contract implementation without economic. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
36316ab to
1f11e69
Compare
| g.decimals = 8 | ||
|
|
||
| desc := native.NewDescriptor("symbol", smartcontract.StringType) | ||
| md := native.NewMethodAndPrice(g.Symbol, 0, callflag.NoneFlag) |
There was a problem hiding this comment.
g.Symbol, g.Decimals and g.TotalSupply methods should be private.
| desc = native.NewDescriptor("transfer", smartcontract.BoolType, | ||
| append(transferParams, manifest.NewParameter("data", smartcontract.AnyType))..., | ||
| ) | ||
| md = native.NewMethodAndPrice(g.Transfer, 1<<17, callflag.States|callflag.AllowCall|callflag.AllowNotify) |
|
|
||
| // BalanceOf returns native GAS token balance for the acc. | ||
| func (g *GAS) BalanceOf(d *dao.Simple, acc util.Uint160) *big.Int { | ||
| return big.NewInt(DefaultBalance * native.GASFactor) |
There was a problem hiding this comment.
| return big.NewInt(DefaultBalance * native.GASFactor) | |
| return big.NewInt(DefaultBalance) |
Because now DefaultBalance is 100 * native.GASFactor.
| type GAS struct { | ||
| interop.ContractMD | ||
| symbol string | ||
| decimals int64 | ||
| } |
There was a problem hiding this comment.
| type GAS struct { | |
| interop.ContractMD | |
| symbol string | |
| decimals int64 | |
| } | |
| type GAS struct { | |
| interop.ContractMD | |
| } |
Keep it as simple as possible. Return constants from symbol and decimals.
| stackitem.NewByteArray(from.BytesBE()), | ||
| stackitem.NewBigInteger(amount), |
There was a problem hiding this comment.
| stackitem.NewByteArray(from.BytesBE()), | |
| stackitem.NewBigInteger(amount), | |
| stackitem.Make(from.BytesBE()), | |
| stackitem.Make(amount), |
Shorter, simpler.
| sig, ok := vectorRaw[j].Value().([]byte) | ||
| if !ok { | ||
| panic(fmt.Errorf("unexpected %d signature value in %d signatures vector: %s expected, %s given", j, i, stackitem.ByteArrayT, sigsVectorsRaw[j].Type())) | ||
| } |
| sigVectors = append(sigVectors, vector) | ||
| } | ||
|
|
||
| cnrListRaw := ic.DAO.GetStorageItem(m.ID, append([]byte{containerPlacementPrefix}, cID[:]...)) |
There was a problem hiding this comment.
cID.BytesBE(), ditto everywhere.
| } | ||
|
|
||
| func isSignedBySNs(ic *interop.Context, contract util.Uint160, cID util.Uint256, placementVectorsNumber int) error { | ||
| if l := len(ic.Tx.Scripts); l != 1 { |
There was a problem hiding this comment.
| if l := len(ic.Tx.Scripts); l != 1 { | |
| if l := len(ic.Tx.Signers); l != 1 { |
For proper RPC test invocations work.
| } | ||
|
|
||
| acc := hash.Hash160(verifScript(contract, cID, placementVectorsNumber)) | ||
| if !ic.Tx.Signers[0].Account.Equals(acc) { |
There was a problem hiding this comment.
| if !ic.Tx.Signers[0].Account.Equals(acc) { | |
| if !ic.Tx.Sender().Equals(acc) { |
| golang.org/x/mod v0.32.0 // indirect | ||
| golang.org/x/text v0.34.0 // indirect | ||
| golang.org/x/tools v0.41.0 // indirect |
Contract part of #3742.