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

Sender from signers #1752

Merged
merged 30 commits into from
Jul 11, 2020
Merged

Sender from signers #1752

merged 30 commits into from
Jul 11, 2020

Conversation

shargon
Copy link
Member

@shargon shargon commented Jul 7, 2020

Close #1747

If we are all agree I can fix the UT

Copy link
Member

@erikzhang erikzhang left a comment

Choose a reason for hiding this comment

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

If so, the Signers is not an optional field, and should be moved from the Attributes.

@shargon
Copy link
Member Author

shargon commented Jul 7, 2020

If so, the Signers is not an optional field, and should be moved from the Attributes.

Agree, what do you prefer @erikzhang ?

@Tommo-L
Copy link
Contributor

Tommo-L commented Jul 7, 2020

If so, could move Witness into Signer?

@erikzhang
Copy link
Member

If so, could move Witness into Signer?

Of course not.

@shargon
Copy link
Member Author

shargon commented Jul 7, 2020

@roman-khimov do you think that it will improve if Signers are moved outside Attributes?

@roman-khimov
Copy link
Contributor

roman-khimov commented Jul 7, 2020

@roman-khimov do you think that it will improve if Signers are moved outside Attributes?

Well, we're losing a byte this way for signers array length, but we're also gaining it for signer attribute type, so in the case of a single Sender it doesn't change anything from the resulting transaction's length point of view. But if we're to have more signers, we'll have to add an excessive byte for signer attribute type for each sender and it will make attribute-based serialization scheme a bit less efficient. Also, a transaction has to have a Sender anyway and attributes are considered to be optional in general, so putting something mandatory there seems a bit wrong. And they could also be interleaved with other attributes which just doesn't look good.

So I think moving them outside of attributes makes for a more clear picture: every transaction has Senders, they're ordered (matching the respective witnesses) and there is nothing else there like some other attributes. And attributes are left for some optional additional functionality.

@erikzhang
Copy link
Member

Let's move them out.

@vncoelho
Copy link
Member

vncoelho commented Jul 8, 2020

If so, the Signers is not an optional field, and should be moved from the Attributes.

I agree with this and with the array.

@shargon
Copy link
Member Author

shargon commented Jul 8, 2020

Please take a look before fix the UT

@Tommo-L Tommo-L mentioned this pull request Jul 9, 2020
16 tasks
@erikzhang
Copy link
Member

Need to add a new flag to WitnessScope. None or FeeOnly?

@shargon
Copy link
Member Author

shargon commented Jul 9, 2020

FeeOnly it's good

@erikzhang
Copy link
Member

erikzhang commented Jul 10, 2020

You are right. The first can be anything.

@shargon
Copy link
Member Author

shargon commented Jul 11, 2020

@roman-khimov could you check it?

@shargon shargon merged commit c4799e9 into neo-project:master Jul 11, 2020
@shargon shargon deleted the sender-from-signers branch July 11, 2020 09:24
@ProDog
Copy link
Contributor

ProDog commented Jul 14, 2020

TransactionAttribute still useful?🤔

@erikzhang
Copy link
Member

It is used in Oracle.

shargon added a commit that referenced this pull request Jul 22, 2020
* Allow call from native contract (#1700)

* Fix MethodCallback (#1723)

* Fix #1714 (#1715)

* Add base64 SYSCALLs (#1717)

* Neo.VM.3.0.0-CI00230 (#1725)

* Allow to iterate buffer (#1726)

* Buffer iterator

* Rename

* Remove using

Co-authored-by: Erik Zhang <[email protected]>

* Update StorageContext.cs (#1728)

StorageContext needs to be public so it can be accessed by Neo Debugger

Co-authored-by: Erik Zhang <[email protected]>

* Fix return value check (#1730)

Co-authored-by: Shargon <[email protected]>

* Fix ContractEventDescriptor (#1733)

* fix enumerator (#1744)

Co-authored-by: Tommo-L <[email protected]>

* Change json fields to all lower case for consistency (#1736)

* Replace DataCache.Find by DataCache.Seek (#1740)

* replace DataCache.Find by DataCache.Seek

* fix order

* optimize

* Update ByteArrayComparer.cs

* Update ByteArrayComparer.cs

* Update DataCache.cs

* fix comments

* fix comments

* fix comments

* Update DataCache.cs

* Update DataCache.cs

* Reorder methods

Co-authored-by: Tommo-L <[email protected]>
Co-authored-by: erikzhang <[email protected]>
Co-authored-by: Shargon <[email protected]>

* Add MaxVerificationGas (#1745)

Co-authored-by: Shargon <[email protected]>

* Create KeyBuilder (#1748)

* Speed up the initialization of ApplicationEngine. (#1749)

* Change nef checksum to double SHA256 (#1751)

* Change to double SHA256

* Clean code

* Revert change

* Clean code

* Fix checksum

* Update NefFile.cs

* Fix compile

* Clean code

Clean code
Fix checksum
Update NefFile.cs
Fix compile

* Optimize

* Optimize

* Fix

Co-authored-by: erikzhang <[email protected]>

* Check witnesses on isStandard (#1754)

* Check witness on isStandard

* Add IsDeployed

* Add IsPayable

* Fix UT

* format

* Add coverage

* Remove IsPayable, IsDeployed

* Move NEP10 to manifest (#1729)

* NEP10 abi

* To lower case

* Move to Manifest

* Clean code

* Update ContractManifest.cs

* Update ContractManifest.cs

* Fix native contracts

Co-authored-by: Erik Zhang <[email protected]>

* Capture fault Exception (#1761)

* Capture faultException

* Update Wallet error

* Remove flag check

* Clean code

* Sender from signers (#1752)

* Sender from signers

* Remove co-

* Move signers outside attributes

* Fix UT and remove Signers class

* Fix UT

* Add FeeOnly scope

* Remove orderBy

* Remove _signersCache

* Fix Signers

* Fix WitnessScope

* Fix Sender

* Update TransactionAttributeType.cs

* Update Wallet.cs

* Fix Wallet

* Rename

* Update Wallet.cs

* Update Wallet.cs

* Partial UT fix

* More UT fixes

* Fix Sender's WitnessScope

* Fix Wallet

* Fix UT

* Explicit FeeOnly for DeployNativeContracts

* Same order as serialization

* Test FeeOnly

* dotnet format

* format

Co-authored-by: Erik Zhang <[email protected]>

* IApplicationEngineProvider (#1758)

* Remove the lock from SendersFeeMonitor and rename it to TransactionVerificationContext (#1756)

* Add EffectiveVoterTurnout (#1762)

* Remove AllowedTriggers from SYSCALLs (#1755)

* fix validatorscount (#1770)

Co-authored-by: Tommo-L <[email protected]>

* impl SeekInternal

Co-authored-by: Erik Zhang <[email protected]>
Co-authored-by: Shargon <[email protected]>
Co-authored-by: Harry Pierson <[email protected]>
Co-authored-by: Luchuan <[email protected]>
Co-authored-by: Tommo-L <[email protected]>
Co-authored-by: joeqian <[email protected]>
shargon added a commit that referenced this pull request Jul 23, 2020
* Allow call from native contract (#1700)

* Fix MethodCallback (#1723)

* Fix #1714 (#1715)

* Add base64 SYSCALLs (#1717)

* Neo.VM.3.0.0-CI00230 (#1725)

* Allow to iterate buffer (#1726)

* Buffer iterator

* Rename

* Remove using

Co-authored-by: Erik Zhang <[email protected]>

* Update StorageContext.cs (#1728)

StorageContext needs to be public so it can be accessed by Neo Debugger

Co-authored-by: Erik Zhang <[email protected]>

* Fix return value check (#1730)

Co-authored-by: Shargon <[email protected]>

* Fix ContractEventDescriptor (#1733)

* fix enumerator (#1744)

Co-authored-by: Tommo-L <[email protected]>

* Change json fields to all lower case for consistency (#1736)

* Replace DataCache.Find by DataCache.Seek (#1740)

* replace DataCache.Find by DataCache.Seek

* fix order

* optimize

* Update ByteArrayComparer.cs

* Update ByteArrayComparer.cs

* Update DataCache.cs

* fix comments

* fix comments

* fix comments

* Update DataCache.cs

* Update DataCache.cs

* Reorder methods

Co-authored-by: Tommo-L <[email protected]>
Co-authored-by: erikzhang <[email protected]>
Co-authored-by: Shargon <[email protected]>

* Add MaxVerificationGas (#1745)

Co-authored-by: Shargon <[email protected]>

* Create KeyBuilder (#1748)

* Speed up the initialization of ApplicationEngine. (#1749)

* Change nef checksum to double SHA256 (#1751)

* Change to double SHA256

* Clean code

* Revert change

* Clean code

* Fix checksum

* Update NefFile.cs

* Fix compile

* Clean code

Clean code
Fix checksum
Update NefFile.cs
Fix compile

* Optimize

* Optimize

* Fix

Co-authored-by: erikzhang <[email protected]>

* Check witnesses on isStandard (#1754)

* Check witness on isStandard

* Add IsDeployed

* Add IsPayable

* Fix UT

* format

* Add coverage

* Remove IsPayable, IsDeployed

* Move NEP10 to manifest (#1729)

* NEP10 abi

* To lower case

* Move to Manifest

* Clean code

* Update ContractManifest.cs

* Update ContractManifest.cs

* Fix native contracts

Co-authored-by: Erik Zhang <[email protected]>

* Capture fault Exception (#1761)

* Capture faultException

* Update Wallet error

* Remove flag check

* Clean code

* Sender from signers (#1752)

* Sender from signers

* Remove co-

* Move signers outside attributes

* Fix UT and remove Signers class

* Fix UT

* Add FeeOnly scope

* Remove orderBy

* Remove _signersCache

* Fix Signers

* Fix WitnessScope

* Fix Sender

* Update TransactionAttributeType.cs

* Update Wallet.cs

* Fix Wallet

* Rename

* Update Wallet.cs

* Update Wallet.cs

* Partial UT fix

* More UT fixes

* Fix Sender's WitnessScope

* Fix Wallet

* Fix UT

* Explicit FeeOnly for DeployNativeContracts

* Same order as serialization

* Test FeeOnly

* dotnet format

* format

Co-authored-by: Erik Zhang <[email protected]>

* IApplicationEngineProvider (#1758)

* Remove the lock from SendersFeeMonitor and rename it to TransactionVerificationContext (#1756)

* Add EffectiveVoterTurnout (#1762)

* Remove AllowedTriggers from SYSCALLs (#1755)

* fix validatorscount (#1770)

Co-authored-by: Tommo-L <[email protected]>

* workflows: use checkout action v2 (#1775)

* Update git workflow

* Update .github/workflows/main.yml

Co-authored-by: Erik Zhang <[email protected]>

Co-authored-by: Erik Zhang <[email protected]>

* Add cache to native contract executions V2 (#1766)

* cache v2

* More optimizations

* Policy contract optimization

* Remove interporable variable

* Two more

* Add Set

* Optimizations

* Optimize

Co-authored-by: erikzhang <[email protected]>

* Optimize attributes (#1774)

* Add length before compression data (#1768)

* Add length before compression

* Optimize

* Fix UT

* Add UT

* Add UT

Co-authored-by: erikzhang <[email protected]>

* Fix VerifyWitnesses (#1776)

* Ensure non predictable peers (#1739)

* Plugins from List to array

* Move false to init

* Fix UT

* Refactor

* Add Exception Message For CreateContract (#1787)

* Add Exception Message When Create Contract

* Add Exception Message

* Code optimization

* add UT

Co-authored-by: Shargon <[email protected]>
Co-authored-by: Qiao Jin <[email protected]>

Co-authored-by: Erik Zhang <[email protected]>
Co-authored-by: Shargon <[email protected]>
Co-authored-by: Harry Pierson <[email protected]>
Co-authored-by: Luchuan <[email protected]>
Co-authored-by: Tommo-L <[email protected]>
Co-authored-by: joeqian <[email protected]>
Co-authored-by: cloud8little <[email protected]>
Co-authored-by: Qiao Jin <[email protected]>
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Jul 30, 2020
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Aug 4, 2020
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Aug 4, 2020
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Aug 4, 2020
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request Aug 4, 2020
@ryanfalzon
Copy link

Can someone please update the documentation to reflect changes done in this pull request. Haver been tyring to understand how to use the new TransactionManager but have been receiving error 500 Invalid from the JSON RPC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge transaction's Sender and Cosigners on the wire
7 participants