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

Add nonce validation to trusted calls #225

Merged
merged 33 commits into from
Apr 6, 2021
Merged

Add nonce validation to trusted calls #225

merged 33 commits into from
Apr 6, 2021

Conversation

haerdib
Copy link
Contributor

@haerdib haerdib commented Mar 29, 2021

closes #89
and some bug fixes (unit tests & block.rs went missing somewhere in the merging process..)

@haerdib haerdib requested a review from brenzi March 29, 2021 09:22
Copy link
Collaborator

@brenzi brenzi left a comment

Choose a reason for hiding this comment

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

you have added the two files block.rsand test.rsto this PR although they are actually missing commits from the previsous PR. this is not good. please make them their own PR which we will merge first, with their own commit message

@haerdib haerdib marked this pull request as draft March 31, 2021 10:02
* add trusted getter for nonce

* exchange get_nonce client function with trusted getter

* remove obsolete system api

* remove pool validation (state must not be loaded for every tx)
@haerdib
Copy link
Contributor Author

haerdib commented Mar 31, 2021

All issues above should now be fixed in my opinion. The only thing missing is your proposal of the local storage of the nonce within the client. I opened issue #231 accordingly. Should we treat this as an seperate issue or do you want this to be included in this PR first?

@haerdib haerdib marked this pull request as ready for review March 31, 2021 15:20
@brenzi
Copy link
Collaborator

brenzi commented Apr 6, 2021

@haerdib can you confirm that this closes #89?

stf/src/sgx.rs Outdated
} else {
debug!("sender balance is zero");
ext.execute_with(|| {
let mut root_account = AccountId::default();
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do you use the term root if it isn't meant as superuser? Why don't you clone the account in-place?

Copy link
Collaborator

Choose a reason for hiding this comment

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

or call it sender instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@haerdib can you confirm that this closes #89?

Yes, I confirm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed with commit 7f33b2a

stf/src/sgx.rs Outdated
let result = match call.call {
TrustedCall::balance_set_balance(root, who, free_balance, reserved_balance) => {
root_account = root.clone();
validate_nonce(&root_account, call.nonce)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why don't you do this before match?
It is a convention that the first argument of a TrustedCall is the caller's accountid, so you don't actually need to know what call it is to extract the accountid I guess?

Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if the problem is different number of arguments for enum values.....is there a idiomatic solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed with commit 7f33b2a

@brenzi brenzi merged commit 1b18c35 into sidechain Apr 6, 2021
haerdib added a commit that referenced this pull request Apr 6, 2021
* closes #89 
* Trusted getter for nonce (#232)
brenzi pushed a commit that referenced this pull request Apr 8, 2021
* closes #89 
* Trusted getter for nonce (#232)
brenzi pushed a commit that referenced this pull request Apr 8, 2021
* closes #89 
* Trusted getter for nonce (#232)
@haerdib haerdib deleted the add-nonce branch April 16, 2021 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants