Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
2ffef77
Candidate 2
quantumagi Mar 30, 2021
b286f79
Fix tests
quantumagi Apr 6, 2021
5abe3b8
Copy contract to output
quantumagi Apr 6, 2021
8d02279
Update comments
quantumagi Apr 8, 2021
c0264b1
Add signatory management
quantumagi Apr 8, 2021
bf6e443
Remove unused struct
quantumagi Apr 8, 2021
5018f77
Update tests
quantumagi Apr 8, 2021
b2f092d
Fix typo
quantumagi Apr 8, 2021
d5f27cd
Add VerifySignatures private method
quantumagi Apr 8, 2021
d7e5d4d
Refactor
quantumagi Apr 8, 2021
61cddba
Remove SetQuorum
quantumagi Apr 8, 2021
d550216
Fix commented code
quantumagi Apr 8, 2021
d12435d
Add null checks
quantumagi Apr 8, 2021
77e0ad2
Add signatures argument
quantumagi Apr 12, 2021
b7036e4
Refactor
quantumagi Apr 12, 2021
95df108
Update comments
quantumagi Apr 13, 2021
874f26b
Update contract
quantumagi Apr 14, 2021
5bb2101
Merge branch 'infracontracts' into candidate2
quantumagi Apr 15, 2021
62641be
Merge branch 'infracontracts' into candidate2
quantumagi Apr 15, 2021
a7f457c
Add VerifySignatures and update test cases
quantumagi Apr 15, 2021
aa9794d
Changes based on feedback
quantumagi Apr 16, 2021
bd66981
Changes based on feedback
quantumagi Apr 16, 2021
7b76017
Changes based on feedback
quantumagi Apr 16, 2021
dcded24
Changes based on feedback
quantumagi Apr 16, 2021
bec8fff
Add namespace
quantumagi Apr 16, 2021
5695579
Update test
quantumagi Apr 16, 2021
d2f02ad
Changes based on feedback
quantumagi Apr 16, 2021
d08549c
Modify signaturure verification method
quantumagi Apr 17, 2021
02357c2
Revert changes
quantumagi Apr 17, 2021
523d4bd
Revert changes
quantumagi Apr 17, 2021
6b8e48d
Changes based on feedback
quantumagi Apr 18, 2021
6e5e1f7
Refactor
quantumagi Apr 18, 2021
5333622
Merge master
quantumagi Apr 19, 2021
8d0d17e
Merge branch 'infracontracts' into candidate2
quantumagi Apr 19, 2021
ce850ee
Merge branch 'infracontracts' into candidate2
quantumagi Apr 19, 2021
a535af1
Add comments to contract
quantumagi Apr 19, 2021
3ad0954
Add comment
quantumagi Apr 19, 2021
b659b86
Update constructor
quantumagi Apr 22, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/Stratis.SmartContracts.CLR.Tests/AuctionTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,11 @@ public class TestPersistentState : IPersistentState
private T GetObject<T>(string key)
{
if (this.objects.ContainsKey(key))
return (T)this.objects[key];
{
object value = this.objects[key];
if (value != null)
return (T)value;
}

return default(T);
}
Expand Down Expand Up @@ -212,7 +216,7 @@ public T GetStruct<T>(string key) where T : struct

public T[] GetArray<T>(string key)
{
throw new NotImplementedException();
return (T[])this.GetObject<T[]>(key).Clone();
}

public void SetBytes(byte[] key, byte[] value)
Expand Down Expand Up @@ -288,12 +292,12 @@ public void SetStruct<T>(string key, T value) where T : struct

public void SetArray(string key, Array a)
{
throw new NotImplementedException();
this.SetObject(key, a);
}

public void Clear(string key)
{
throw new NotImplementedException();
this.SetBytes(key, null);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,288 @@
using Stratis.SmartContracts;
using ECRecover=Stratis.SCL.Crypto.ECRecover;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you import just namespace in here ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These type of using statements for fixing conflicts when one type is exist in more than 2 namespace so this is not a case in here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm... It would protect against any conflicts that may arise in the future?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems fine.


public struct WhiteListEntry
{
public UInt256 CodeHash;
public Address LastAddress;
public string Name;
}

/// <summary>
/// The primary purpose is to provide a mechanism by which smart contracts are whitelisted
/// and secondly to maintain the list of authenticators (signatories) required to do so.
/// </summary>
/// <remarks>
/// All method calls that change the whitelist or signatories require multiple signatories
/// as specified by the current list of defined signatories and the quorum requirement.
/// </remarks>
public class SystemContractsDictionary : SmartContract
{
const string primaryGroup = "main";

/// <summary>
/// Initializes the contract with the default (or initial) set of signatories which are required when calling some of the methods.
/// </summary>
/// <param name="state">Contract state.</param>
/// <param name="signatories">Initial set of signatories.</param>
/// <param name="quorum">Initial quorum requirement/</param>
public SystemContractsDictionary(ISmartContractState state, Address[] signatories, uint quorum) : base(state)
{
this.SetSignatories(primaryGroup, signatories);
this.SetQuorum(primaryGroup, quorum);
}

private Address[] Signatories => GetSignatories(primaryGroup);

private uint Quorum => GetQuorum(primaryGroup);

private void VerifySignatures(byte[] signatures, string authorizationChallenge)
{
string[] sigs = this.Serializer.ToArray<string>(signatures);

Assert(ECRecover.TryGetVerifiedSignatures(sigs, authorizationChallenge, this.Signatories, out var verifieds), "Invalid signatures");

Assert(verifieds.Length >= this.Quorum, $"Please provide {this.Quorum} valid signatures for '{authorizationChallenge}'.");
}

public Address[] GetSignatories(string group)
{
Assert(!string.IsNullOrEmpty(group));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This assert is not necessary so you can remove it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does make a difference by throwing an error versus simply returning the default. Intuitively throwing an error makes more sense but perhaps there is some reason you don't want to do that for smart contracts. @rowandh , wdyt?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You might save a small amount of gas by doing this. Seems fine.

return this.State.GetArray<Address>($"Signatories:{group}");
}

private void SetSignatories(string group, Address[] values)
{
this.State.SetArray($"Signatories:{group}", values);
}

public uint GetQuorum(string group)
{
Assert(!string.IsNullOrEmpty(group));
return this.State.GetUInt32($"Quorum:{group}");
}

private void SetQuorum(string group, uint value)
{
this.State.SetUInt32($"Quorum:{group}", value);
}

private uint GetGroupNonce(string group)
{
return this.State.GetUInt32($"GroupNonce:{group}");
}

private void SetGroupNonce(string group, uint value)
{
this.State.SetUInt32($"GroupNonce:{group}", value);
}

public void AddSignatory(byte[] signatures, string group, Address address, uint newSize, uint newQuorum)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Who is going to call this method ? Owner of the contract or anyone ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Anyone that's willing to pay the gas cost to execute it. However signatures are checked before any updates are made.

{
Assert(!string.IsNullOrEmpty(group));
Assert(newSize >= newQuorum, "The number of signatories can't be less than the quorum.");

Address[] signatories = this.GetSignatories(group);
foreach (Address signatory in signatories)
Assert(signatory != address, "The signatory already exists.");

Assert((signatories.Length + 1) == newSize, "The expected size is incorrect.");

This comment was marked as resolved.

@quantumagi quantumagi Apr 10, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Equality check should not be negated in here?

It is correct afaik. The error is displayed when the assertion fails.


// The nonce is used to prevent replay attacks.
uint nonce = this.GetGroupNonce(group);

// Validate or provide a unique challenge to the signatories that depends on the exact action being performed.
// If the signatures are missing or fail validation contract execution will stop here.
this.VerifySignatures(signatures, $"{nameof(AddSignatory)}(Nonce:{nonce},Group:{group},Address:{address},NewSize:{newSize},NewQuorum:{newQuorum})");

System.Array.Resize(ref signatories, signatories.Length + 1);
signatories[signatories.Length - 1] = address;

this.SetSignatories(group, signatories);
this.SetQuorum(group, newQuorum);
this.SetGroupNonce(group, nonce + 1);
}

public void RemoveSignatory(byte[] signatures, string group, Address address, uint newSize, uint newQuorum)
{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These methods can be called by anyone ?

@quantumagi quantumagi Apr 15, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Anyone that's willing to pay the gas cost to execute it. However signatures are checked before any updates are made.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is really needed to specify newSize and newQuorum, why not to automate it ?

@quantumagi quantumagi Apr 16, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I really want the user to be very exact about the change being accomplished especially due to how important the quorum count is in relation to the signatory count. The user must know exactly what is being accomplished in that regards and show that knowledge by specifying the values explicitly. All the signatories will also see that information in the security challenge when signing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hımm but the transaction is going to fail and cost fee if anyone going to call these methods in same block or in prev blocks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand what you're saying. Will mull it over.

Assert(!string.IsNullOrEmpty(group));
Assert(newSize >= newQuorum, "The number of signatories can't be less than the quorum.");

Address[] prevSignatories = this.GetSignatories(group);
Address[] signatories = new Address[prevSignatories.Length - 1];

int i = 0;
foreach (Address item in prevSignatories)
{
if (item == address)
{
continue;
}

Assert(signatories.Length != i, "The signatory does not exist.");

signatories[i++] = item;
}

Assert(newSize == signatories.Length, "The expected size is incorrect.");

// The nonce is used to prevent replay attacks.
uint nonce = this.GetGroupNonce(group);

// Validate or provide a unique challenge to the signatories that depends on the exact action being performed.
// If the signatures are missing or fail validation contract execution will stop here.
this.VerifySignatures(signatures, $"{nameof(RemoveSignatory)}(Nonce:{nonce},Group:{group},Address:{address},NewSize:{newSize},NewQuorum:{newQuorum})");

this.SetSignatories(group, signatories);
this.SetQuorum(group, newQuorum);
this.SetGroupNonce(group, nonce + 1);
}

public bool IsWhiteListed(UInt256 codeHash)
{
Assert(codeHash != default(UInt256));

WhiteListEntry whiteListEntry = this.GetWhiteListEntry(codeHash);

return whiteListEntry.CodeHash != default(UInt256);
}

public UInt256 GetCodeHash(string name)
{
Assert(!string.IsNullOrEmpty(name));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Imo these asserts are not necessary. If someone send null,empty or none existence key then we return default value already from state.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does make a difference by throwing an error versus simply returning the default. Intuitively throwing an error makes more sense but perhaps there is some reason you don't want to do that for smart contracts. @rowandh , wdyt?

@YakupIpek YakupIpek Apr 16, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As caller of the method, i should to verify that there is hash code or not for given name parameter in any case and i do it by checking its value is default or not. But you added assertion, in the case of it is null so i have to verify exception + default value check.

We should not optimize code for exceptional cases.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is common way we did in other contracts ussually.

@quantumagi quantumagi Apr 16, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If there is insufficient checks something like "Quorum:" could obtain a value, then when "Quorum:" is accessed later a junk value could be returned.

so i have to verify exception + default value check.

You don't have to verify exception if you're not passing null values. If you are unexpectedly passing null values then it better to get an error than some junk value you may proceed processing with.


return this.State.GetUInt256($"ByName:{name}");
}

private void SetCodeHash(string name, UInt256 codeHash)
{
this.State.SetUInt256($"ByName:{name}", codeHash);
}

private void ClearCodeHash(string name)
{
this.State.Clear($"ByName:{name}");
}

public Address GetContractAddress(string name)
{
UInt256 codeHash = GetCodeHash(name);

if (codeHash == default(UInt256))
return default(Address);

WhiteListEntry whiteListEntry = this.GetWhiteListEntry(codeHash);

return whiteListEntry.LastAddress;
}

private WhiteListEntry GetWhiteListEntry(UInt256 codeHash)
{
return this.State.GetStruct<WhiteListEntry>($"CodeHash:{codeHash}");
}

private void SetWhiteListEntry(UInt256 codeHash, WhiteListEntry whiteListEntry)
{
this.State.SetStruct<WhiteListEntry>($"CodeHash:{codeHash}", whiteListEntry);
}

private void ClearWhiteListEntry(UInt256 codeHash)
{
this.State.Clear($"CodeHash:{codeHash}");
}

public Address GetContractAddress(UInt256 codeHash)
{
Assert(codeHash != default(UInt256));

return this.GetWhiteListEntry(codeHash).LastAddress;
}

private uint GetCodeNonce(UInt256 codeHash)
{
return this.State.GetUInt32($"Nonce:{codeHash}");
}

private void SetCodeNonce(UInt256 codeHash, uint nonce)
{
this.State.SetUInt32($"Nonce:{codeHash}", nonce);
}

public void WhiteList(byte[] signatures, UInt256 codeHash, Address lastAddress, string name)
{
Assert(signatures != null);
Assert(codeHash != default(UInt256));

UInt256 codeHashKey = codeHash;
WhiteListEntry whiteListEntry;

// If the name already exists then use it to locate the whitelist entry to update.
if (!string.IsNullOrEmpty(name))
{
codeHashKey = this.GetCodeHash(name);

if (codeHashKey == default(UInt256))
codeHashKey = codeHash;
}

whiteListEntry = this.GetWhiteListEntry(codeHashKey);

// The nonce is used to prevent replay attacks.
uint nonce = this.GetCodeNonce(codeHash);

// Validate or provide a unique challenge to the signatories that depends on the exact action being performed.
string authorizationChallenge;
if (whiteListEntry.CodeHash == default(UInt256))
{
authorizationChallenge = $"{nameof(WhiteList)}(Nonce:{nonce},CodeHash:{codeHash},LastAddress:{lastAddress},Name:{name})";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not as below ?

authorizationChallenge = $"{nameof(WhiteList)}:{nonce}:{codeHash},{lastAddress},{name})"

@quantumagi quantumagi Apr 15, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's meant to be as human readable as possible to ensure signatories can read it and know what they are signing.

}
else
{
Assert(whiteListEntry.CodeHash != codeHash || whiteListEntry.LastAddress != lastAddress || whiteListEntry.Name != name, "Nothing changed.");
authorizationChallenge = $"{nameof(WhiteList)}(Nonce:{nonce},CodeHash:{whiteListEntry.CodeHash}=>{codeHash},LastAddress:{whiteListEntry.LastAddress}=>{lastAddress},Name:{whiteListEntry.Name}=>{name})";
}

// If the signatures are missing or fail validation contract execution will stop here.
this.VerifySignatures(signatures, authorizationChallenge);

if (whiteListEntry.CodeHash != default(UInt256))
{
if (codeHash != whiteListEntry.CodeHash)
this.ClearWhiteListEntry(whiteListEntry.CodeHash);

if (string.IsNullOrEmpty(name) && !string.IsNullOrEmpty(whiteListEntry.Name))
this.ClearCodeHash(whiteListEntry.Name);
}

whiteListEntry.CodeHash = codeHash;
whiteListEntry.LastAddress = lastAddress;
whiteListEntry.Name = name;

this.SetWhiteListEntry(codeHash, whiteListEntry);
this.SetCodeHash(name, codeHash);
this.SetCodeNonce(codeHash, nonce + 1);
}

public void BlackList(byte[] signatures, UInt256 codeHash)
{
Assert(signatures != null);
Assert(codeHash != default(UInt256));

WhiteListEntry whiteListEntry = this.GetWhiteListEntry(codeHash);

Assert(whiteListEntry.CodeHash != default(UInt256), "The entry does not exist.");

// The nonce is used to prevent replay attacks.
uint nonce = this.GetCodeNonce(codeHash);

// Validate or provide a unique challenge to the signatories that depends on the exact action being performed.
// If the signatures are missing or fail validation contract execution will stop here.
this.VerifySignatures(signatures, $"{nameof(BlackList)}(Nonce:{nonce},CodeHash:{whiteListEntry.CodeHash},LastAddress:{whiteListEntry.LastAddress},Name:{whiteListEntry.Name})");

this.ClearWhiteListEntry(codeHash);
this.ClearCodeHash(whiteListEntry.Name);

// Don't remove the nonce. Just increment it.
this.SetCodeNonce(codeHash, nonce + 1);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,9 @@
<Compile Update="SmartContracts\StorageTest.cs">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</Compile>
<Compile Update="SmartContracts\SystemContractsDictionary.cs">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</Compile>
<Compile Update="SmartContracts\ThrowExceptionContract.cs">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</Compile>
Expand Down
Loading