Skip to content
This repository has been archived by the owner on Mar 29, 2022. It is now read-only.

Auth method 3 #54

Merged
merged 57 commits into from
Mar 28, 2022
Merged

Auth method 3 #54

merged 57 commits into from
Mar 28, 2022

Conversation

adlerjohn
Copy link
Contributor

Implements the third auth method (owners of all coin inputs are the same).

@adlerjohn adlerjohn added the enhancement New feature or request label Feb 19, 2022
@adlerjohn adlerjohn marked this pull request as draft February 19, 2022 18:28
src/chain/auth.sw Outdated Show resolved Hide resolved
otrho
otrho previously requested changes Feb 21, 2022
Copy link
Contributor

@otrho otrho left a comment

Choose a reason for hiding this comment

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

Some more ASM comments. It's just too tricky to do non-trivial ASM blocks this way. I think we need to rethink how we expect them to be used, perhaps provide a stricter context. Like they must be standalone functions, rather than embedded in other functions, or something.

src/chain/auth.sw Show resolved Hide resolved
src/chain/auth.sw Outdated Show resolved Hide resolved
src/chain/auth.sw Outdated Show resolved Hide resolved
src/chain/auth.sw Outdated Show resolved Hide resolved
@nfurfaro
Copy link
Contributor

@otrho Re: asm
The new code here in the get_coins_owner() function and helper funcs is code that I had stashed before all the comments you made on the is_reentrant()/has_reentered()PR, so this needs to be revised in light of your input there.

@nfurfaro
Copy link
Contributor

This has seen a significant refactor by @adlerjohn , including:

  • refactoring to use the new tx.sw module
  • extra assert() into its own module
  • tests for the auth module in general that I've added.

Still to do:

  • ensure msg_sender() is tested for the case which uses get_coins_owner()

@adlerjohn adlerjohn marked this pull request as ready for review March 28, 2022 20:48
@adlerjohn
Copy link
Contributor Author

Should finally be ready

} else {
let unwrapped = result.unwrap();
if let Sender::ContractId(v) = unwrapped {
assert(v == expected_id);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test isn't perfect because of this assert which shouldn't be here, but the compiler panics if removed. Could be a compiler bug, will investigate later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @otrho

Copy link
Contributor

@nfurfaro nfurfaro Mar 28, 2022

Choose a reason for hiding this comment

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

This is a bit contrived... ideally the test contracts would just pass the Result returned by msg_sender() along to the rust tests and compare the returned values there.
But, I was having issues with contracts returning nested struct/enums (MissingData error when using abigen macro) and so I simplified the return values and make comparisons in the contracts themselves. This is also why we need to pass in the expected_id value.
It feels hacky but works...

Copy link
Contributor

@nfurfaro nfurfaro left a comment

Choose a reason for hiding this comment

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

Looks good. Some improvements to be made in tests, will open an issue to track.

@adlerjohn adlerjohn dismissed otrho’s stale review March 28, 2022 21:05

no more asm!

@adlerjohn adlerjohn merged commit e85b6ce into master Mar 28, 2022
@adlerjohn adlerjohn deleted the furnic/auth-method-3 branch March 28, 2022 21:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants