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 address field to capabilities #736

Merged
merged 8 commits into from
Apr 6, 2021

Conversation

ceelo777
Copy link
Contributor

@ceelo777 ceelo777 commented Mar 28, 2021

Closes #721

Description

This PR should add the ability to grab the address field from a capability.


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@ceelo777
Copy link
Contributor Author

ceelo777 commented Mar 28, 2021

So this isn't finished @turbolent - I'm having a weird issue with the test I wrote -

`
Code Snippet:
func TestCheckCapability_address(t *testing.T) {

  t.Parallel()
  t.Run("check address", func(t *testing.T) {
	  checker, err := ParseAndCheckWithPanic(t,
		  `		  			
			  let capability: Capability = panic("")
			  let addr = capability.address
		  `,
	  )
	  require.NoError(t, err)

	  addrType := RequireGlobalType(t, checker.Elaboration, "addr")
	  require.Equal(t, sema.AddressType{}, addrType)
  })

}
`

I'm getting this error - "global type 'addr' missing" even though it seems that I'm initializing it? Not sure exactly why this is happening.

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Thank you for contributing this!

Looks good, just the test needs to be fixed and then it can be merged

runtime/tests/checker/capability_test.go Outdated Show resolved Hide resolved
@turbolent turbolent self-assigned this Mar 29, 2021
@turbolent turbolent changed the title feature: add address field for capabilities in addition to test Add address field to capabilities Mar 29, 2021
@turbolent
Copy link
Member

@ceelo777 Looks good, glad the test passes now. Just one last thing: Could you please add a test for the interpreter?

@ceelo777
Copy link
Contributor Author

To be honest, I'm not sure if I did this interpreter test correctly but it passed - anything else I should add to it?

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

The test looks good! 👍

Good idea to use the testAccount helper.
One last thing would be to check the result values, then we can merge

runtime/tests/interpreter/capability_test.go Outdated Show resolved Hide resolved
runtime/sema/type.go Outdated Show resolved Hide resolved
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice work, thanks again for adding this @ceelo777!

@turbolent turbolent merged commit 134c59f into onflow:master Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting the Address from a Capability
3 participants