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

[move-lang] Change address value syntax. #8285

Merged
merged 1 commit into from
May 4, 2021
Merged

Conversation

tnowacki
Copy link
Contributor

@tnowacki tnowacki commented Apr 27, 2021

  • Change address value syntax from 0x to @<num>
  • 0x hex values are now numbers
  • Makes room for named addresses

Motivation

  • @ allows for @ syntax for named addresses without shadowing issues from locals or constants

Test Plan

  • New tests

@bors-libra bors-libra added this to In Review in bors Apr 27, 2021
@sblackshear
Copy link
Contributor

Change address value syntax from 0x to @

From the code changes in the PR, it looks like 0x -> @0x--expected?

@tnowacki
Copy link
Contributor Author

Change address value syntax from 0x to @

From the code changes in the PR, it looks like 0x -> @0x--expected

Bad rendering of the grammar comment in github. Should be @<num>. Numbers (u8, u64, u128, or no suffix) can now be hex or decimal, so <num> can be things like 255 or 0xFF. Addresses are then @<num> with no suffix on the number.

@sblackshear
Copy link
Contributor

sblackshear commented Apr 28, 2021

Change address value syntax from 0x to @

From the code changes in the PR, it looks like 0x -> @0x--expected

Bad rendering of the grammar comment in github. Should be @<num>. Numbers (u8, u64, u128, or no suffix) can now be hex or decimal, so <num> can be things like 255 or 0xFF. Addresses are then @<num> with no suffix on the number.

Edit: I think I get it, disregard :). IIUC, you can write either @0x55 or @55, but these correspond to different addresses.

Does that mean this

 public fun DIEM_ROOT_ADDRESS(): address {
        @0xA550C18
 }

could also be written as

public fun DIEM_ROOT_ADDRESS(): address {
        @A550C18
}

@bob-wilson
Copy link
Contributor

could also be written as

public fun DIEM_ROOT_ADDRESS(): address {
        @A550C18
}

No, I don't think so. Without the 0x prefix, the "number" is expected to be a decimal value, and A550C18 is not a valid decimal number. But, you could use @173345816 as the equivalent decimal representation of 0xA550C18.

@tnowacki tnowacki force-pushed the addr branch 2 times, most recently from 7c2b5dd to b27f381 Compare April 30, 2021 22:11
@tnowacki tnowacki marked this pull request as ready for review April 30, 2021 22:18
Copy link
Contributor

@bob-wilson bob-wilson left a comment

Choose a reason for hiding this comment

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

This looks pretty good to me, but I found a few small things to tweak before you land

language/move-lang/src/parser/lexer.rs Outdated Show resolved Hide resolved
language/move-lang/src/parser/syntax.rs Outdated Show resolved Hide resolved
language/move-lang/src/parser/syntax.rs Outdated Show resolved Hide resolved
language/move-lang/src/parser/syntax.rs Outdated Show resolved Hide resolved
language/move-lang/src/parser/syntax.rs Show resolved Hide resolved
language/move-lang/src/shared/mod.rs Show resolved Hide resolved
language/move-lang/src/shared/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bob-wilson bob-wilson left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for the last round of changes

language/move-lang/src/parser/syntax.rs Outdated Show resolved Hide resolved
@tnowacki
Copy link
Contributor Author

tnowacki commented May 4, 2021

/land

@bors-libra bors-libra moved this from In Review to Queued in bors May 4, 2021
@bors-libra bors-libra moved this from Queued to Testing in bors May 4, 2021
bors-libra pushed a commit that referenced this pull request May 4, 2021
- Change address value syntax from 0x to @<num>
- 0x hex values are now numbers
- Makes room for named addresses

Closes: #8285
@sausagee
Copy link
Contributor

sausagee commented May 4, 2021

/cancel

we have a sev3 fix and need to cut the line

@bors-libra bors-libra moved this from Testing to In Review in bors May 4, 2021
@sausagee
Copy link
Contributor

sausagee commented May 4, 2021

/land

@bors-libra bors-libra moved this from In Review to Queued in bors May 4, 2021
- Change address value syntax from 0x to @<num>
- 0x hex values are now numbers
- Makes room for named addresses

Closes: diem#8285
@bors-libra bors-libra moved this from Queued to Testing in bors May 4, 2021
@github-actions
Copy link

github-actions bot commented May 4, 2021

Cluster Test Result

Test runner setup time spent 271 secs
Compatibility test results for land_d5960539 ==> land_6cc08c4f (PR)
1. All instances running land_d5960539, generating some traffic on network
2. First full node land_d5960539 ==> land_6cc08c4f, to validate new full node to old validator node traffic
3. First Validator node land_d5960539 ==> land_6cc08c4f, to validate storage compatibility
4. First batch validators (14) land_d5960539 ==> land_6cc08c4f, to test consensus and traffic between old full nodes and new validator node
5. First batch full nodes (14) land_d5960539 ==> land_6cc08c4f
6. Second batch validators (15) land_d5960539 ==> land_6cc08c4f, to upgrade rest of the validators
7. Second batch of full nodes (15) land_d5960539 ==> land_6cc08c4f, to finish the network upgrade, time spent 674 secs
all up : 869 TPS, 5228 ms latency, 5950 ms p99 latency, no expired txns, time spent 250 secs
Logs: http://kibana.ct-1-k8s-testnet.aws.hlw3truzy4ls.com/app/kibana#/discover?_g=(time:(from:'2021-05-04T23:16:02Z',to:'2021-05-04T23:38:51Z'))
Dashboard: http://grafana.ct-1-k8s-testnet.aws.hlw3truzy4ls.com/d/performance/performance?from=1620170162000&to=1620171531000
Validator 1 logs: http://kibana.ct-1-k8s-testnet.aws.hlw3truzy4ls.com/app/kibana#/discover?_g=(time:(from:'2021-05-04T23:16:02Z',to:'2021-05-04T23:38:51Z'))&_a=(columns:!(log),query:(language:kuery,query:'kubernetes.pod_name:"val-1"'),sort:!(!('@timestamp',desc)))

Repro cmd:

./scripts/cti --tag land_d5960539 --cluster-test-tag land_6cc08c4f -E BATCH_SIZE=15 -E UPDATE_TO_TAG=land_6cc08c4f --report report.json --suite land_blocking_compat

🎉 Land-blocking cluster test passed! 👌

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.

None yet

5 participants