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

Documentation update for 2.x #956

Merged
merged 35 commits into from
May 19, 2020
Merged

Documentation update for 2.x #956

merged 35 commits into from
May 19, 2020

Conversation

roman-khimov
Copy link
Member

Problem

Stale or completely missing documentation for interops, compiler and RPC.

Solution

Update it a little.

It also fixes some interops, compiler and fixes #914.

There is a Neo.Account.IsStandard syscall, but we didn't have a wrapper for
it.
Turns out, they never functioned correctly.
@roman-khimov roman-khimov added compiler Go smart contract compiler documentation Improvements or additions to documentation rpc RPC server and client labels May 18, 2020
@roman-khimov roman-khimov added this to the v0.75.0 milestone May 18, 2020
@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

Merging #956 into master-2.x will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master-2.x     #956   +/-   ##
===========================================
  Coverage       68.20%   68.20%           
===========================================
  Files             144      144           
  Lines           14186    14186           
===========================================
  Hits             9675     9675           
  Misses           4058     4058           
  Partials          453      453           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ddbc905...ddbc905. Read the comment docs.

// GetVotes returns current votes of the given account represented as a slice of
// public keys. Keys are serialized into byte slices in their compressed form (33
// bytes long each). This function uses `Neo.Account.GetVotes` syscall
// internally.
func GetVotes(a Account) [][]byte {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about making this interops a method of an Account struct. Not here, of course.

Copy link
Member Author

Choose a reason for hiding this comment

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

#296. And #941. But it's not a priority for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

And not for 2.x also, I think.

pkg/interop/asset/asset.go Show resolved Hide resolved
// Create registers a new asset on the blockchain (similar to old Register
// transaction). `assetType` parameter has the same set of possible values as
// GetAssetType result, `amount` must be multiplied by 10⁸, `precision` limits
// the smallest possible amount of new Asset to 10⁻ⁿ (where n in precision which
Copy link
Contributor

Choose a reason for hiding this comment

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

in -> is or remove which

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK

pkg/interop/attribute/attribute.go Show resolved Hide resolved

// GetHeight returns the height of te block recorded in the current execution scope.
// GetHeight returns current block height (index of the last accepted block,
// note that transaction in being run as a part of new block this block is
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an unpaired parenthesis and I don't understand what does this mean. Probably being run as a part of new block is superfluous and this can be split in several sentences, to make it easier to read.

Copy link
Member Author

@roman-khimov roman-khimov May 19, 2020

Choose a reason for hiding this comment

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

I'll probably rephrase it a little.

pkg/interop/crypto/crypto.go Show resolved Hide resolved
pkg/interop/storage/storage.go Show resolved Hide resolved
pkg/interop/util/util.go Show resolved Hide resolved
docs/rpc.md Outdated

Both methods also don't currently support arrays in function parameters.
This server accepts websocket connections on `$BASE_URL/ws` address. You can
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it worth mentioning URI scheme: ws://$BASE_URL/ws

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK

* global variables can't be changed in functions (#638)
* it's not possible to rename imported interop packages, they won't work this
way (#397, #913)

Copy link
Contributor

Choose a reason for hiding this comment

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

Are nested selectors going to be supported? From my experience I often want to use them when cycling slice of structures, but can't. If not, then it is worth mention it there.

for i := 0; i < len(arr); i++ {
    foo(arr[i].bar)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Please file an issue for it (it should be fixed eventually) and I'll add it to this list.

Copy link
Contributor

Choose a reason for hiding this comment

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

#957

Also don't forget that range loops with value variable are also not supported.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, #958.

Both Create and Renew have things returned from them.
GetTransaction never worked with hash, it works with indexes.
It should return keys, attempting to push []*state.Validator to the stack
would probably lead to failure.
And sort syscall names as they change the indentation anyway.
It interferes with interop/blockchain and it's not a default directory for
chain DBs.
This syscall should only work for contracts created by current transaction and
that is what is supposed to be checked here. Do so by looking at the
differences between ic.dao and original lower DAO.
Some functions were just not correct in their interfaces.
Previously we could generate dynamic appcall with a kludge of
    AppCall([]byte{/* 20 zeroes */, realScriptHash, args...)

Now there is a separate function for this.
We have a syscall for it, so it should be exposed.
Notify doesn't return anything!
There is no such syscall as Neo.Transaction.GetScript and GetScript should be
available for contract's use.
It's useless. Even though there is Neo.Transaction.GetUnspentCoins syscall
that can be used, its return type is an interop structure that's not accepted
by any other syscall, so you can't really do anything with it. And there is no
such interface for the .net Framework.
We have now way better godoc for interop functions, so this document makes
little sense and it's not referenced anywhere, so it's safe to drop it.
And add one more reference to it into the main README.
@roman-khimov roman-khimov merged commit f0d6b0a into master-2.x May 19, 2020
@roman-khimov roman-khimov deleted the doc-update-2.x branch May 19, 2020 13:02
This was referenced May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler Go smart contract compiler documentation Improvements or additions to documentation rpc RPC server and client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants