-
Notifications
You must be signed in to change notification settings - Fork 226
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
spike: fusdc to evm and solana #10967
base: master
Are you sure you want to change the base?
Conversation
chainAddress.chainId.startsWith('noble') || | ||
Fail`'depositForBurn' not supported on chain: ${q(chainAddress.chainId)}`; |
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.
This won't work for testnets, as noble's is grand-1
. Seems like the ecosystem
field we were talking about, or even something in addition to that, on Chain
would be helpful here.
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 think we're going to have to ask the ChainHub for info about the address.
// TODO: logic for encoding solana + evm addresses | ||
// see https://github.com/circlefin/noble-cctp/blob/master/examples/depositForBurn.ts#L52-L70 | ||
const mintRecipient = new Uint8Array(); |
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.
Feeding an LLM this example code + some data from noble -> solana txs, it claims we need something like this:
const B58_ALPHABET = '123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz';
// For Solana addresses - decode base58 address into bytes
function base58AddrToBytes(addr) {
let result = 0n;
for (let i = 0; i < addr.length; i++) {
result = result * 58n + BigInt(B58_ALPHABET.indexOf(addr[i]));
}
// Convert to bytes
const bytes = [];
while (result > 0n) {
bytes.unshift(Number(result & 0xffn));
result = result >> 8n;
}
// Add leading zeros from original encoding
let leadingZeros = 0;
for (let i = 0; i < addr.length && addr[i] === '1'; i++) {
leadingZeros++;
}
return new Uint8Array([...new Array(leadingZeros).fill(0), ...bytes]);
}
// For Ethereum addresses - encode hex address into bytes
function hexAddrToBytes(addr) {
// Remove 0x prefix if present and ensure lowercase
const hex = addr.replace(/^0x/, '').toLowerCase();
// Pad to 32 bytes (64 chars) with leading zeros
const padded = hex.padStart(64, '0');
// Convert to bytes
const bytes = new Uint8Array(32);
for (let i = 0; i < 32; i++) {
bytes[i] = parseInt(padded.substr(i * 2, 2), 16);
}
return bytes;
}
We might prefer to use libraries here, suggestions welcome. For b58 I believe there is base-x
: CI['chainId'] extends `noble${string}` | ||
? CosmosChainAccountMethods<CI> & NobleMethods |
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.
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.
seems like we could use the presence of cctpDestinationDomain
.
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.
cctpDestinationDomain
i agree
// TODO FIX: | ||
// 1. withdraw advanceAmount from poolAccount | ||
// 2. returnToPool |
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.
this bug was fixed in a different PR that has since been merged to master
6912ab5
to
2b54d03
Compare
I rebased on master because #10988 basically covers the first commit here. |
@@ -143,6 +143,8 @@ export type AddressHook = { | |||
query: { | |||
/** end user destination address */ | |||
EUD: string; | |||
/** chain id for end user destination. necessary if EUD is not 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.
in CAIP-10 parlance,
CID
: chain_id
(à la CAIP-2)
EUD
: account_address
It also has an account_id
definition (chain_id + ":" + account_address
). I propose the "End User Destination" be that full "account_id". The enclosing object is a "query" and it's not sensical to change CID independent of EUD. IOW, EUD needs to specify the full chain-agnostic account ID.
For backwards compatibility we can make the chain_id
implied.
* chainId to an integer. | ||
* @param {string} chainId | ||
*/ | ||
const formatChainId = chainId => { |
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.
why not just leave it as a string? there's no arithmetic being performed.
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 eip155 chains, chain ID is uint
, so the thinking was to preserve that
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 know of a current use case, but could imagine a smart contract function signature that expects an integer for a chainId parameter. Perhaps these types of functions can handle this concern themselves instead of the orchestration api.
For eth ecosystem chain ids, let's please at least use {number}
for the type value.
closes: #XXXX
refs: #XXXX
Description
Includes:
.toProtoMsg
codegenChain
takesKnownChainName
generic parameter instead ofChainInfo
#10213cosmos-orchestration-account.js
hasdepositForBurn
method (wip).depositForBurn
(wip)Todo:
chainId.startsWith('noble')
is insufficient for determining if an account should have.depositForBurn
(via types + run time checks), as noble's testnet is calledgrand-1
. (comment)depositForBurn
fails during advancesettler.forward()
logic for fallback transfers (should includedepositForBurn
flow)multichain-testing
testsSecurity Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations