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.

Sign up for GitHub

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

Jump to bottom

upd/Neo-go to v0.103.0 #2635

Merged
merged 3 commits into from
Nov 7, 2023
Merged

upd/Neo-go to v0.103.0 #2635

merged 3 commits into from
Nov 7, 2023

Conversation

carpawell
Copy link
Member

No description provided.

@codecov Codecov
Copy link

codecov bot commented Oct 31, 2023 •
edited
Loading

Codecov Report

Merging #2635 (b031597) into master (ca78d31) will increase coverage by 0.00%.
Report is 1 commits behind head on master.
The diff coverage is 0.00%.

❗ Current head b031597 differs from pull request most recent head 6da2ec2. Consider uploading reports for the commit 6da2ec2 to get more accurate results

@@           Coverage Diff           @@
##           master    #2635   +/-   ##
=======================================
  Coverage   29.21%   29.21%           
=======================================
  Files         416      416           
  Lines       31328    31330    +2     
=======================================
+ Hits         9152     9154    +2     
- Misses      21376    21377    +1     
+ Partials      800      799    -1
Files Coverage Δ
cmd/neofs-adm/internal/modules/morph/notary.go 0.00% <0.00%> (ø)
pkg/morph/client/notary.go 0.00% <0.00%> (ø)
pkg/morph/deploy/notary.go 0.00% <0.00%> (ø)
cmd/neofs-adm/internal/modules/morph/config.go 0.00% <0.00%> (ø)
...d/neofs-adm/internal/modules/morph/local_client.go 0.00% <0.00%> (ø)
cmd/neofs-adm/internal/modules/morph/policy.go 0.00% <0.00%> (ø)
...d/neofs-adm/internal/modules/morph/renew_domain.go 0.00% <0.00%> (ø)
...-adm/internal/modules/morph/initialize_register.go 0.00% <0.00%> (ø)
cmd/neofs-adm/internal/modules/morph/balance.go 0.00% <0.00%> (ø)
...md/neofs-adm/internal/modules/morph/dump_hashes.go 0.00% <0.00%> (ø)
... and 2 more

... and 1 file with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@carpawell carpawell marked this pull request as ready for review November 1, 2023 11:49
@carpawell carpawell self-assigned this Nov 1, 2023
@carpawell carpawell requested a review from AnnaShaleva November 2, 2023 13:19
AnnaShaleva
AnnaShaleva reviewed Nov 3, 2023
View reviewed changes
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Is there any other places where emit NeoGo package is uses for custom scripts creation instead of smartcontract's Builder? If so, then we should also adjust them (if possible).

cmd/neofs-adm/internal/modules/morph/initialize_transfer.go Outdated
Amount: initialProxyGASAmount,
from := c.CommitteeAcc.Contract.ScriptHash()

tx, err := createNEP17MultiTransferTx(c.Client, c.CommitteeAcc, gas.Hash, []nep17.TransferParameters{{
Copy link
Member

Choose a reason for hiding this comment

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

Maybe then createNEP17MultiTransferTx can be removed completely? And you transfer it from the committee account anyway, so you can use committee actor directly from c.CommitteeAct without creating simple actor. Is there any pitfalls?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe then createNEP17MultiTransferTx can be removed completely?

can be done, ok

Is there any pitfalls?

do not think there are any

Copy link
Member

Choose a reason for hiding this comment

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

If we've agreed to implement Builder-related changes in a separate PR, then let's fix this issue, and otherwise LGTM.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe then createNEP17MultiTransferTx can be removed completely?

fixed

@carpawell
Copy link
Member Author

Is there any other places where emit NeoGo package is uses for custom scripts creation instead of smartcontract's Builder? If so, then we should also adjust them (if possible).

There are some for sure. @roman-khimov, do we need it? How long neofs-adm gonna live (or be unchanged) after we merge auto deploy?

@roman-khimov
Copy link
Member

neofs-adm gives much more abilities than just a deploy and we need some low-level instruments as well, so it won't disappear tomorrow.

@carpawell
Copy link
Member Author

neofs-adm gives much more abilities than just a deploy and we need some low-level instruments as well

neofs-amd without deploying commands is neofs-cli to me.

If so, then we should also adjust them (if possible).

Done.

@carpawell carpawell requested a review from AnnaShaleva November 7, 2023 15:46
AnnaShaleva
AnnaShaleva reviewed Nov 7, 2023
View reviewed changes
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

LGTM otherwise.

cmd/neofs-adm/internal/modules/morph/balance.go
emit.AppCall(w.BinWriter, nnsCs.Hash, "resolve", callflag.ReadOnly,
getAlphabetNNSDomain(i),
int64(nns.TXT))
b.InvokeMethod(nnsCs.Hash, "resolve", getAlphabetNNSDomain(i), int64(nns.TXT))
Copy link
Member

Choose a reason for hiding this comment

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

BTW, builder invokes this with callflag.All, and it was ReadOnly before. But anyway, it's our contract and there's no harm in it.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, yeah, forgot to ping you about that. @roman-khimov, yes, in some cases there were readonly calls for readonly operations but nothing should break because of it. tbh, i really do not see much difference b/w InvokeMethod and AppCall except the call flags (and sometimes we need them)

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 OK to have this RO -> ALL replacement in Adm.

Copy link
Member

Choose a reason for hiding this comment

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

I've seen this, it doesn't matter much for these calls, most of them are test invocations only.

cmd/neofs-adm/internal/modules/morph/balance.go Outdated Show resolved Hide resolved
@roman-khimov roman-khimov merged commit 6b077ad into master Nov 7, 2023
8 checks passed
@roman-khimov roman-khimov deleted the upd/neo-go-103 branch November 7, 2023 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roman-khimov roman-khimov roman-khimov approved these changes

@AnnaShaleva AnnaShaleva AnnaShaleva approved these changes

@cthulhu-rider cthulhu-rider Awaiting requested review from cthulhu-rider cthulhu-rider is a code owner

@AliceInHunterland AliceInHunterland Awaiting requested review from AliceInHunterland

Assignees

@carpawell carpawell

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@carpawell @roman-khimov @AnnaShaleva