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 Bridge example #36

Merged
merged 2 commits into from
Mar 28, 2024
Merged

Conversation

notV4l
Copy link
Collaborator

@notV4l notV4l commented Jan 15, 2024

No description provided.

Copy link
Contributor

@glihm glihm left a comment

Choose a reason for hiding this comment

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

First review to have more context.
Will take the time to run everything later in the day.

Good job on the makefiles and documentation though.

Comment on lines +154 to +155
let token_dispatcher = IERC20BridgeableDispatcher { contract_address: data.l2_token };
token_dispatcher.burn(caller, amount);
Copy link
Contributor

Choose a reason for hiding this comment

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

When messaging is involved, I would opt for escrow over burning directly. If anything goes wrong in the process, reverting an escrow is not affecting the liquidity the same way as burning/minting tokens.

But the escrow definitely adds more logic to handle, so having a process to mint them again is perhaps completely fine in this scenario.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tokens are escrowed on L1 & mint/ burn on L2 & total_supply remains constant

fn l2_bridge_address(self: @TState,) -> ContractAddress;
fn mint(ref self: TState, recipient: ContractAddress, amount: u256);

// WITHOUT INTERFACE !!!
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by that?

Copy link
Collaborator Author

@notV4l notV4l Jan 17, 2024

Choose a reason for hiding this comment

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

this is codegen it just say next fn are not included in another interface
https://github.com/notV4l/origami/blob/bridge_exemple/token/scripts/genInterface.js
it generate interface from artifact (i used it when composing components to check exposed fn)
maybe cainome could handle this

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, if we could centralize codegen it could be awesome. With cainome, when plugin with stdin are supported, you have in fact a list of organized tokens instead of parsing the raw ABI that can be easily used to such tasks.

But I think that for now it's fine using this script to not delay this PR as tt's a specific use case which we have to see how it can be integrated in cainome for easy use.
Something like this from a rust code:

let trait_string = AbiParser::abi_to_trait("TraitName", manifest.abi);

examples/bridge/sn/src/dojo_bridge.cairo Show resolved Hide resolved
examples/bridge/sn/src/dojo_bridge.cairo Outdated Show resolved Hide resolved
@@ -0,0 +1,14 @@
## DOJO BRIDGE
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## DOJO BRIDGE
## Dojo Bridge

Copy link
Contributor

Choose a reason for hiding this comment

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

we should use submodules and no embed these

Comment on lines +69 to +77
use token::components::security::initializable::initializable_component;

use token::components::token::erc20::erc20_metadata::erc20_metadata_component;
use token::components::token::erc20::erc20_balance::erc20_balance_component;
use token::components::token::erc20::erc20_allowance::erc20_allowance_component;
use token::components::token::erc20::erc20_mintable::erc20_mintable_component;
use token::components::token::erc20::erc20_burnable::erc20_burnable_component;
use token::components::token::erc20::erc20_bridgeable::erc20_bridgeable_component;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
use token::components::security::initializable::initializable_component;
use token::components::token::erc20::erc20_metadata::erc20_metadata_component;
use token::components::token::erc20::erc20_balance::erc20_balance_component;
use token::components::token::erc20::erc20_allowance::erc20_allowance_component;
use token::components::token::erc20::erc20_mintable::erc20_mintable_component;
use token::components::token::erc20::erc20_burnable::erc20_burnable_component;
use token::components::token::erc20::erc20_bridgeable::erc20_bridgeable_component;
use token::components::{
security::initializable::initializable_component,
token::erc20::{
erc20_metadata::erc20_metadata_component,
erc20_balance::erc20_balance_component,
erc20_allowance::erc20_allowance_component,
erc20_mintable::erc20_mintable_component,
erc20_burnable::erc20_burnable_component,
erc20_bridgeable::erc20_bridgeable_component,
},
};

@ponderingdemocritus
Copy link
Contributor

@notV4l could you finish these last few tasks

@ponderingdemocritus ponderingdemocritus merged commit 001cd3d into dojoengine:main Mar 28, 2024
7 checks passed
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