Skip to content

Aj/contracts#63

Open
vibhurajeev wants to merge 49 commits into
mainfrom
aj/contracts
Open

Aj/contracts#63
vibhurajeev wants to merge 49 commits into
mainfrom
aj/contracts

Conversation

@vibhurajeev

Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread contracts/src/GethImageID.sol Outdated
pragma solidity ^0.8.20;

library ImageID {
bytes32 public constant ADAPTER_ID = bytes32(0x35e11aa083da4b62f64a80c2b731097e5dcf9b0608a9689cd10b4633d56dcc7a);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

actually we need this when we are using the nexus manager contract with ethereum mock geth adapter.

Comment thread contracts/src/NexusProofManager.sol Outdated
contract NexusProofManager {
uint256 public latestNexusBlockNumber = 0;
RiscZeroVerifierRouter public immutable risc0Router;
bytes32 public constant imageId = ImageID.ADAPTER_ID; // added for the auto-generated contract

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Can we make this configurable? Only guardian should be able to update nexus image ID

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there is no guardian config in this contract. Adding this.

Comment thread contracts/src/NexusProofManager.sol Outdated
}
// TODO : include a verification check after finalization
// ! Do not use this code in production.
availBridgeRootToAvailHeight[bridgeRoot] = availBlockNumber;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We need to store this against nexus block number, so we are able to retrieve it with getChainState. AppAddress is 0 hash.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in doc it is written that :

Stores the avail bridge root, mapped to avail height. Got confused let's discuss this on call

Comment thread contracts/src/NexusProofManager.sol Outdated
availBridgeRootToAvailHeight[bridgeRoot] = availBlockNumber;
}

function getChainState(uint256 blockNumber, bytes32 nexusAppID) external view returns (bytes32) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Can we rename all AppID to AppAddress?

Comment thread contracts/src/ZksyncImageID.sol Outdated

pragma solidity ^0.8.20;

library ImageID {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

We do not need this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need this when running the nexus manager with zksync adapter.

uint256
) external view returns (bytes32);
function nexusAppIDToLatestBlockNumber(bytes32) external view returns (uint256);
function nexusAppIDToState(bytes32, uint256) external view returns (bytes32);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

this is same as getChainState, right?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If it is, good to remove this in the same PR so we do not forget later

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

updated this.

@@ -12,19 +12,12 @@ library JellyfishMerkleTreeVerifier {
bytes32[] siblings;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Skipping checking this file, as I assume it is only lint changes

@@ -0,0 +1,123 @@
// SPDX-License-Identifier: Apache-2.0

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not checking this file. Please confirm if you have checked that it works.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes a test is added.

Comment thread contracts/test/GethElf.sol Outdated

library Elf {
string public constant ADAPTER_PATH =
"/Users/ocdbytes/Desktop/Avail/nexus/target/riscv-guest/geth-methods/adapter/riscv32im-risc0-zkvm-elf/release/adapter";

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Do we need this for the tests?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes we would need this

Comment thread contracts/test/ZksyncElf.sol Outdated

library Elf {
string public constant ZKSYNC_ADAPTER_PATH =
"/Users/ocdbytes/Desktop/Avail/nexus/target/riscv-guest/zksync-methods/zksync-adapter/riscv32im-risc0-zkvm-elf/release/zksync-adapter";

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Just confirming if we need this as well

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

needed.

Comment thread core/src/state/vm_state.rs Outdated
}
let proof = tree.get_with_proof(KeyHash(key.as_fixed_slice().clone()), version)?;

println!(">>> root hash : {:?}", jmt::RootHash(root.as_fixed_slice().clone()));

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Can be removed or changed to tracing debug

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Also same for all below similar lines

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

done

Comment thread core/src/state/vm_state.rs Outdated
let proof = tree.get_with_proof(KeyHash(key.as_fixed_slice().clone()), version)?;

println!(">>> root hash : {:?}", jmt::RootHash(root.as_fixed_slice().clone()));
println!(">>> key hash : {:?}", hex::encode(key.as_fixed_slice().clone()));

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Same as above.

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.

3 participants