-
Notifications
You must be signed in to change notification settings - Fork 94
cli: db download-contract command #3960
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?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -17,16 +17,23 @@ import ( | |||||||||||||
"github.com/nspcc-dev/neo-go/pkg/core" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/core/block" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/core/chaindump" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/core/dao" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/core/native" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/core/state" | ||||||||||||||
corestate "github.com/nspcc-dev/neo-go/pkg/core/stateroot" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/core/storage" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/core/transaction" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/interop/native/management" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/io" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/network" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/rpcclient" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/services/metrics" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/services/notary" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/services/oracle" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/services/rpcsrv" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/services/stateroot" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/util" | ||||||||||||||
"github.com/nspcc-dev/neo-go/pkg/vm/stackitem" | ||||||||||||||
"github.com/urfave/cli/v2" | ||||||||||||||
"go.uber.org/zap" | ||||||||||||||
"go.uber.org/zap/zapcore" | ||||||||||||||
|
@@ -82,6 +89,27 @@ func NewCommands() []*cli.Command { | |||||||||||||
Usage: "Height of the state to reset DB to", | ||||||||||||||
Required: true, | ||||||||||||||
}) | ||||||||||||||
var cfgDownloadFlags = slices.Clone(cfgFlags) | ||||||||||||||
cfgDownloadFlags = append(cfgDownloadFlags, options.RPC...) | ||||||||||||||
cfgDownloadFlags = append(cfgDownloadFlags, | ||||||||||||||
&cli.UintFlag{ | ||||||||||||||
Name: "height", | ||||||||||||||
Usage: "Height at which to get the contract state for", | ||||||||||||||
Required: false, | ||||||||||||||
DefaultText: "latest", | ||||||||||||||
}, | ||||||||||||||
&cli.StringFlag{ | ||||||||||||||
Name: "contract-hash", | ||||||||||||||
Usage: "Script hash of the contract to download", | ||||||||||||||
Required: true, | ||||||||||||||
Aliases: []string{"c"}, | ||||||||||||||
}, | ||||||||||||||
|
&flags.AddressFlag{ | |
Name: "contract", | |
Aliases: []string{"c"}, | |
Required: true, | |
Usage: "Contract hash or address", | |
}, |
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 say that this command is closer to contract
subset of commands (e.g. ./neo-go contract download
). Also, it reminds me of ./neo-go wallet import-deployed
, so for the sake of CLI interface unification we can use it as ./neo-go contract import-deployed
. But it's a matter of discussion because, as @ixje said, this commend performs a direct DB modification.
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.
performs a direct DB modification
Which makes it a db
command for me. But I'm thinking of separating download and import stages (consider contract state as well). In which case contract download
+ db import-contract
would be a nice pair. This separation can be useful for unit tests similar to ones we have in neofs-contract repository where some state is stored in the repository and imported as needed for tests.
This needs to be discussed, it is possible to unify these tools and we better do this now, before we have 25 variations on the same thing.
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.
Vote up for separation.
Outdated
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 code will be simplified after flag type replacement.
Outdated
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.
Useless blank line.
Outdated
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.
Comment should start with a capital letter and end with a period.
Outdated
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.
Not critical, but chain.GetContractScriptHash
is sufficient here.
Outdated
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.
We need to check two cases: if err == nil
means that ID is occupied, we can take the next free one. if err != nil && !errors.Is(err, storage.ErrKeyNotFound)
means that it's likely some internal DB error, we should exit in this case.
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.
Native cache is tighten to DAO, native.PutContractState
will panic here because native Management is trying to update its cache whereas cache wasn't initialized for newly-created DAO d
.
I don't quite like the fact that we use non-initialized newly-created DAO d
for this operation. I'd say that the ideal way is to use wrapped/private DAO inherited from chain's DAO:
Line 104 in 4340c76
func (dao *Simple) GetPrivate() *Simple { |
With this approach native cache will work properly and we won't need any workarounds to disable it. The problem is that this approach requires additional getter defined on Blockchain
to expose chain's DAO which is also not the best solution. Requires @roman-khimov ACK.
An alternative I thought of - move contract insertion code to the Blockchain
itself, like any other methods that perform direct chain's state modification (jumpToState
, resetState
, etc.)
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.
@AnnaShaleva, check neofs-contract code, it solved a similar problem IIRC.
Outdated
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.
Won't work with the current implementation, because Management uses cache to retrieve state, and cache is not updated.
So I'd suggest to remove this call, it doesn't carry any functional meaning.
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 was a leftover
Outdated
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.
Don't escape double quotes, wrap the string into backticks (`) instead.
Outdated
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.
Use fmt.Fprintf(ctx.App.Writer, ...)
.
Outdated
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.
Use single line for 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.
Once options.Historic
flag is used, use GetRPCWithInvoker
, it will return historic Invoker and then...
Outdated
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.
nativehashes.ContractManagement
.
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.
... we can get rid of this code. Just invoke native Management's getContract
via historic Invoker, that's it.
Using internal knowledge of native Management storage like []byte{0x08}
at upper-level modules is not the best idea, it should be avoided if we can. The drawback of the suggested approach is: it won't work with C# node, they don't support historic invocations. @ixje is it critical?
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.
Critical no, but might be inconvenient as now we'll need to add some graceful handling if the RPC node doesn't support historic fetching.
How about adding a comment explaining the prefix
used while maintaining support for both types of nodes? It is not as if this is likely to ever change
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, let's stick to the implementation-agnostic solution.
How about adding a comment
Agree, it's needed.
Also, let's replace magic 0x08
byte with native.PrefixContract
constant, so that it will be more clear to other developers.
Outdated
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.
Use single line.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -802,6 +802,13 @@ func PutContractState(d *dao.Simple, cs *state.Contract) error { | |
return putContractState(d, cs, true) | ||
} | ||
|
||
// TODO: check with nspcc if we can update PutContractState with an extra param | ||
// or else how to have the cache for ManagementContract initialized such that it doesn't panic | ||
// trying to run markUpdated() | ||
func PutContractStateNoCache(d *dao.Simple, cs *state.Contract) error { | ||
return putContractState(d, cs, false) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the comment above at |
||
|
||
// putContractState is an internal PutContractState representation. | ||
func putContractState(d *dao.Simple, cs *state.Contract, updateCache bool) error { | ||
key := MakeContractKey(cs.Hash) | ||
|
@@ -860,3 +867,17 @@ func checkScriptAndMethods(ic *interop.Context, script []byte, methods []manifes | |
|
||
return nil | ||
} | ||
|
||
// TODO: decide how to best expose. There are too many options that for the time I picked a duplicate | ||
// function to have something working and let nspcc decide their preferred exposing option | ||
func GetNextContractID(d *dao.Simple) (int32, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's firstly decide where to place DB modification code. |
||
si := d.GetStorageItem(ManagementContractID, keyNextAvailableID) | ||
if si == nil { | ||
return 0, errors.New("nextAvailableID is not initialized") | ||
} | ||
id := bigint.FromBytes(si) | ||
ret := int32(id.Int64()) | ||
id.Add(id, intOne) | ||
d.PutBigInt(ManagementContractID, keyNextAvailableID, id) | ||
return ret, nil | ||
} |
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.
We have
options.Historic
designed specifically for such kind of things.