-
Notifications
You must be signed in to change notification settings - Fork 37
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
Implemented Address Computer #423
Conversation
@@ -1,11 +1,10 @@ | |||
import { WasmVirtualMachine } from "../constants"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fine, we can consider that this isn't a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why this could be breaking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being a publicly exported element, a movement can affect clients that use fully qualified imports (in this context, we can simply think of this as a non-breaking change, though).
this.numberOfShardsWithoutMeta = numberOfShardsWithoutMeta || CURRENT_NUMBER_OF_SHARDS_WITHOUT_META; | ||
} | ||
|
||
computeContractAddress(deployer: IAddress, deploymentNonce: bigint): Address { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call this implementation from smartContract.ts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
src/address.ts
Outdated
} | ||
|
||
computeContractAddress(deployer: IAddress, deploymentNonce: bigint): Address { | ||
let initialPadding = Buffer.alloc(8, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
where possible. Here, and below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
return this.getShardOfPubkey(address.getPublicKey(), this.numberOfShardsWithoutMeta); | ||
} | ||
|
||
private getShardOfPubkey(pubkey: Uint8Array, numberOfShards: number): number { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For review, linking to the Go and PY implementations:
- https://github.com/multiversx/mx-chain-core-go/blob/main/core/sharding/multiShardCoordinator.go#L52
- https://github.com/multiversx/mx-sdk-py-core/blob/main/multiversx_sdk_core/address.py#L141
This works for 1, 2, 3 and 4 shards. But will fail for more. See the Go implementation. For the moment, we can keep the PY and JS implementations as they are, but we should fix them in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will replicate the Go implementation in the future.
|
||
contractAddress = addressComputer.computeContractAddress(deployer, 1n); | ||
assert.equal(contractAddress.toHex(), "000000000000000005006e4f90488e27342f9a46e1809452c85ee7186566bd5e"); | ||
assert.equal(contractAddress.toBech32(), "erd1qqqqqqqqqqqqqpgqde8eqjywyu6zlxjxuxqfg5kgtmn3setxh40qen8egy"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/smartcontracts/smartContract.ts
Outdated
|
||
let address = new Address(addressBytes); | ||
return address; | ||
const deployer = new Address(owner.bech32()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could have used the new approach, Address.fromBech32()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed.
Implemented the
AddressComputer
class according to the sdk-specs.